Opened 7 years ago

Closed 7 years ago

#2679 closed defect (fixed)

use value-param tests for database tests, not type-param tests

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130219
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 10.90 Internal?: no

Description

I suggest switching from type parameterized tests (TYPED_TEST) to
value parameterized tests (TEST_P) for
datasrc/tests/database_unittest.

Type parameterized tests are C++-templated heavily and requires a lot
of build time and sometimes even cause build failure (as a result of
requesting larger memory). This will be even heavier when we support
other databases.

FYI: auth query_unittest uses value parameterized tests to avoid this trap.

Subtickets

Change History (15)

comment:1 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130219

comment:2 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:3 Changed 7 years ago by jinmei

trac2679 is ready for review.

I know the diff is big and looks scary, but this is basically
straightforward refactoring (trust me:-).

This is suggested guide for the review:

  • the first 2 commits (up to b68752a) are initial preparation, mainly consisting of trivial keyword substitution from TYPED_TEST to TEST_P or removing this (which is not requested in parameterized tests). Since the diff so far is big but it's quite trivial, I suggest reviewing this part first, separately.
  • next 5 commits (905792e^..f866170) complete the conversion. The important changes are the first and last commits. These include some non trivial tricks to adjust the test fixture with the parameter framework instead of the template. The intermediate 3 (905792e..b20cc15) should be trivial editorial cleanup. My suggestion is to review these 5 commits as a set, concentrating on the diff for the first and last commits.

If the branch looks too big at this point, we can stop and defer the
rest to another ticket. In case the rest of the branch is deemed to
be acceptable: the remaining part is to reorganize the test fixture so
it will be more easily shared with other accessor implementations.
Specifically, test fixtures classes were extracted into a separate
header file (newly created), and the SQLite3 tests were now moved to
another separate file (as an example of how to do it for other
accessors).

  • the first commit in this part (b2ac76a) is big, but it only moves the class definitions to the separate header file, and rearranged the rest of the code so it compiles successfully. So, in essence, this should be a straightforward update. I suggest reviewing this diff separately.
  • the next 3 commits (b2ac76a..4f5d524) are to move SQLite3 related part into a separate file. This should also be straightforward.
  • The rest (4694c98 and beyond) is documentation and other cleanups.

This change shouldn't need a changelog entry; it's just an internal
refactoring for tests.

comment:4 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:5 Changed 7 years ago by jinmei

Additional note: I believe we can close #1272 with this ticket
(this branch removed the "workaround" mentioned in #1272).

comment:6 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I found some minor things in the branch. I hope I didn't miss anything important in the large amount of diff, but it probably couldn't have been made shorter.

Do the commits 79f65e1240e0104f6d425ce63821e1297b78a3ce and 4792e513f5dc087a6dc5e51f72acf0efd0cc4f37 contain anything but reindentation and removal of the #if 0 lines?

Is this really true?

/// This is similar to TEST_RECORDS, but the first entry is base32-hex encoded
/// hash value (which is expected to appear as the first label of NSEC3
/// owner names), not an FQDN.

If so, what is the purpose of the split in the load function? That split seems to expect a name and takes the first label, but if it's only the hash, it should be passed as it is, shouldn't it?

Also, I was surprised it is possible to instantiate the tests multiple times with different values each time and have only one set of the test bodies. Is that really supported by GTEST, or does it work by accident?

It really seems #1272 can be closed.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by jinmei

Thanks for reviewing the big diff!

Replying to vorner:

I found some minor things in the branch. I hope I didn't miss anything important in the large amount of diff, but it probably couldn't have been made shorter.

Do the commits 79f65e1240e0104f6d425ce63821e1297b78a3ce and 4792e513f5dc087a6dc5e51f72acf0efd0cc4f37 contain anything but reindentation and removal of the #if 0 lines?

To be very accurate, there are some other things, but all are
editorial/style matters anyway:

  • constify
  • position of ++

Is this really true?

/// This is similar to TEST_RECORDS, but the first entry is base32-hex encoded
/// hash value (which is expected to appear as the first label of NSEC3
/// owner names), not an FQDN.

It's true, but to be specific in case it was misunderstood:

TEST_RECORDS[][0] = "www.example.org." // and so on
TEST_NSEC3_RECORDS[][0] = "0P9MHAVEQVM6T7VBL5LOP2U3T2RP3TOM" // and so on

...but, I don't understand the rest of the comment. What's the load
function?

If so, what is the purpose of the split in the load function? That split seems to expect a name and takes the first label, but if it's only the hash, it should be passed as it is, shouldn't it?

Also, I was surprised it is possible to instantiate the tests multiple times with different values each time and have only one set of the test bodies. Is that really supported by GTEST, or does it work by accident?

Are you asking about that we have this in database_unittest.cc

INSTANTIATE_TEST_CASE_P(, DatabaseClientTest, ::testing::Values(&mock_param));

and this in database_sqlite3_unittest.cc?

INSTANTIATE_TEST_CASE_P(SQLite3, DatabaseClientTest,
                        ::testing::Values(&sqlite3_param));

If so, I believe this clarifies it:
http://code.google.com/p/googletest/wiki/V1_5_AdvancedGuide#Creating_Value-Parameterized_Abstract_Tests

It really seems #1272 can be closed.

Okay, thanks.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Thanks for reviewing the big diff!

Most of it was just very simple replacement, nothing to think about. So it just took slightly longer time.

TEST_RECORDS[][0] = "www.example.org." // and so on
TEST_NSEC3_RECORDS[][0] = "0P9MHAVEQVM6T7VBL5LOP2U3T2RP3TOM" // and so on

...but, I don't understand the rest of the comment. What's the load
function?

The enableNSEC3Generic function. I mean this code:

    // Add NSEC3s
    for (int i = 0; TEST_NSEC3_RECORDS[i][0] != NULL; ++i) {
        const string nsec3_columns[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT] =
            {
                Name(TEST_NSEC3_RECORDS[i][0]).split(0, 1).toText(true),
                TEST_NSEC3_RECORDS[i][2], // TTL
                TEST_NSEC3_RECORDS[i][1], // RR type
                TEST_NSEC3_RECORDS[i][4]  // RDATA
            };
        accessor.addNSEC3RecordToZone(nsec3_columns);
    }

Specially, the line:

                Name(TEST_NSEC3_RECORDS[i][0]).split(0, 1).toText(true),

It seems to expect the string is actually a full name, not just the first label (or I don't understand the purpose of the split there).

comment:11 in reply to: ↑ 10 Changed 7 years ago by jinmei

Replying to vorner:

The enableNSEC3Generic function. I mean this code:

    // Add NSEC3s
    for (int i = 0; TEST_NSEC3_RECORDS[i][0] != NULL; ++i) {
        const string nsec3_columns[DatabaseAccessor::ADD_NSEC3_COLUMN_COUNT] =
            {
                Name(TEST_NSEC3_RECORDS[i][0]).split(0, 1).toText(true),
                TEST_NSEC3_RECORDS[i][2], // TTL
                TEST_NSEC3_RECORDS[i][1], // RR type
                TEST_NSEC3_RECORDS[i][4]  // RDATA
            };
        accessor.addNSEC3RecordToZone(nsec3_columns);
    }

Specially, the line:

                Name(TEST_NSEC3_RECORDS[i][0]).split(0, 1).toText(true),

It seems to expect the string is actually a full name, not just the first label (or I don't understand the purpose of the split there).

Ah, okay, good catch. Although it's not a fault of this branch but
an existing oddity, that conversion wasn't necessary. I guess it was
a result of naive incorporation from the main database.cc
implementation (but it's been working for either case by luck). I
just simplified it.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:13 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 2.40

Hello

I think it is ready for merge now.

comment:14 in reply to: ↑ 13 Changed 7 years ago by jinmei

Replying to vorner:

Hello

I think it is ready for merge now.

Thanks, merge done, closing.

comment:15 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 2.40 to 10.90
Note: See TracTickets for help on using tickets.