Table based tests for importers #897

sebdah · 2017-07-25T15:25:44Z

What does this do / why do we need it?

The current tests for the importers (like godep or glide) have lots of similarities. Using table based tests would reduce code and make it easier to add more test cases.

What should your reviewer look out for in this PR?

I'm adding gps.ProjectRootPtr() as well as gps.RevisionPtr() functions. They are helpers to make it easy to create pointers to ProjectRoot / Revision easily. Since they are only used for tests at the moment, it may be better that I just create them in my _test.go file for now. Suggestions welcome.

Do you need help or clarification on anything?

I'm not entirely satisfied with the amount of lines that ended up in the for loop for the test cases. Ideally I'd use something like testify/assert to reduce the line count, increase readability and make test failure output more descriptive. But I'm not sure that importing dependencies into the project is desirable. Please advice.

To do

Implement table based tests for godep
Implement table based tests for glide
~~Possibly move helpers to their own file. Currently intPtr() lives in godep_importer_test.go, which is not desirable~~

Which issue(s) does this PR fix?

fixes #824

sdboyer

i've not reviewed in detail, but this seems fundamentally 💯 sane. however, we definitely want to use subtests for this! if you've not used them before, just grep for t.Run( in the project - we use them all over the place, plenty of examples to go from.

sdboyer · 2017-07-26T08:48:44Z

cmd/dep/glide_importer_test.go

+	defer sm.Release()
+
+	for name, testCase := range testCases {
+		t.Logf("Running test case: %s", name)


Ideal situation for using subtests, here 😄

sebdah · 2017-07-26T10:14:59Z

Cool, thanks for the review! I haven't used sub tests, but I'll read up and push a fix for that. Cheers

…

On Wed, 26 Jul 2017 at 14:20 sam boyer ***@***.***> wrote: ***@***.**** requested changes on this pull request. i've not reviewed in detail, but this seems fundamentally 💯 sane. however, we definitely want to use subtests for this! if you've not used them before, just grep for t.Run( in the project - we use them all over the place, plenty of examples to go from. ------------------------------ In cmd/dep/glide_importer_test.go <#897 (comment)>: > + }, + projectRoot: testGlideProjectRoot, + expectConvertErr: true, + }, + } + + h := test.NewHelper(t) + defer h.Cleanup() + + ctx := newTestContext(h) + sm, err := ctx.SourceManager() + h.Must(err) + defer sm.Release() + + for name, testCase := range testCases { + t.Logf("Running test case: %s", name) Ideal situation for using subtests, here 😄 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#897 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0QLDJ2urO9inf6S5ldAPB-7v_wcMAzks5sRv3CgaJpZM4OiuFK> .

sebdah · 2017-07-27T08:32:59Z

I've make the changes this morning. But wondering if something is up with CI for the golang/dep builds. The failures seems unrelated to my fix.

sdboyer

gettin pretty close, just a few issues.

sdboyer · 2017-07-28T05:19:30Z

cmd/dep/glide_importer_test.go

+		projectRoot             gps.ProjectRoot
+		sourceRepo              string
+		notExpectedProjectRoot  *gps.ProjectRoot
+		expectedConstraint      string


nit: in Go-land, i've learned we apparently prefer "want" to "expect".

sdboyer · 2017-07-28T05:20:59Z

cmd/dep/godep_importer_test.go

+		expectConvertErr       bool
+		matchPairedVersion     bool
+		projectRoot            gps.ProjectRoot
+		notExpectedProjectRoot *gps.ProjectRoot


why does this need to be a pointer?

I'm having it as a pointer to represent that it's not set. So that we can distinguish between:

A project root "github.com/a/b/c"

An empty project root ""

A test case that does not have a notExpectedProjectRoot test case

In other words: If it's nil it won't be part of the test case, while making it possible to have any other value when we use it for testing.

hmm. the basic argument makes sense, though it still seems like a bit of a code smell. i can't come up with a specific objection, though, so i'm OK to move ahead with it.

sdboyer · 2017-07-28T05:21:35Z

internal/gps/identifier.go

@@ -40,6 +40,11 @@ import (
 // the type system when a path-ish string must have particular semantics.
 type ProjectRoot string

+// ProjectRootPtr takes a ProjectRoot value and returns a pointer.
+func ProjectRootPtr(p ProjectRoot) *ProjectRoot {


if we must have this (and i'm not clear on why we do), can it at least not be exported?

I think we should remove this and make it an unexported part of the tests. Since it's not used outside the tests I think this is not providing value to any part of the implementation.

I'll move it to the test files.

sdboyer · 2017-07-28T05:21:48Z

internal/gps/version.go

@@ -198,6 +198,11 @@ func (r Revision) identical(c Constraint) bool {
 	return r == r2
 }

+// RevisionPtr takes a revision value and return it's pointer.
+func RevisionPtr(r Revision) *Revision {


same question as with ProjectRootPtr()

sebdah · 2017-07-29T04:50:59Z

Thanks, I have updated the tests according to the review feedback!

darkowlzz · 2017-07-29T11:17:13Z

cmd/dep/godep_importer_test.go

@@ -53,7 +236,7 @@ func TestGodepConfig_Import(t *testing.T) {
 		t.Fatal("Expected the lock to be generated")
 	}

-	goldenFile := "godep/expected_import_output.txt"
+	goldenFile := "godep/want_import_output.txt"
 	got := verboseOutput.String()


Filename change 😬 seems to be causing the test to fail. No such file exists. I think it's fine to keep it expected_import_output.txt.

carolynvs · 2017-07-31T15:09:13Z

cmd/dep/godep_importer_test.go

@@ -395,3 +338,13 @@ func equalImports(a, b []godepPackage) bool {

 	return true
 }
+
+// projectRootPtr takes a ProjectRoot value and returns a pointer.
+func projectRootPtr(p gps.ProjectRoot) *gps.ProjectRoot {


Here's how you can deal with these string types (gps.Revision and gps.ProjectRoot) without introducing these helper functions:

Change the want/got in the testcase matrix from pointers to the struct type, e.g. *gps.Revision to gps.Revision.

Update the assignments to not use projectRootPtr/revisionPtr.

Update the asserts to compare the want testcase value against the empty string, instead of nil, e.g.
if testCase.notWantedProjectRoot != "" {

I can do that, if you'd like me to. The reason I have it this way is to be able to represent the difference between e.g. an empty notWantedProjectRoot ("") and a test case without a project root (nil). Otherwise we could not test for the case where notWantedProjectRoot is empty.

Let me know which approach you'd like and I'll adjust accordingly.

i'm on the fence - ordinarily, i'd say that we've tried to architect around situations where the zero could have multiple interpretations. but the importers are tricky, because we're converting other systems' semantics into our own, so it's harder to maintain invariants like that. at the same time, it is just a single, confined test pattern, so i don't know that it really matters too much.

either way, it's your call, @carolynvs 😄

I've laid out in the comments an alternative way to simplify the test cases and remove these helper functions. Let me know if they make sense.

I don't mean to hate on them, but if we can reorganize the tests to not need them, that would make me happy. 😀

Alright! I'll adjust the tests according to your comments @carolynvs. Will to that within a week, I'm currently on vacation. Thanks for the feedback!

carolynvs

Sorry this took me so long to look at!

carolynvs · 2017-08-07T16:40:49Z

cmd/dep/glide_importer_test.go

+					p.Ident().ProjectRoot)
+			}
+
+			if p.Ident().Source != testCase.sourceRepo {


nit: Can we rename this to wantSourceRepo? Just to be consistent with the other fields in the test case.

carolynvs · 2017-08-07T16:45:43Z

cmd/dep/godep_importer_test.go

+				},
+			},
+			projectRoot:          gps.ProjectRoot("github.com/sdboyer/deptest"),
+			notWantedProjectRoot: projectRootPtr(gps.ProjectRoot("github.com/sdboyer/deptest/foo")),


Between the check for an Constraint entry for projectRoot and the check on wantLockCount, I would be quite happy to remove notWantedProjectRoot entirely. It will effectively test what the importer cared about: consolidating multiple subpackages into a single entry for the root package.

Then we can remove projectRootPtr as well. Sound good?

carolynvs · 2017-08-07T16:47:33Z

cmd/dep/glide_importer_test.go

+				t.Fatalf("Expected locked version to be '%s', got %s", testCase.wantVersion, ver)
+			}
+
+			if testCase.wantRevision != nil {


Since we only check wantRevision when matchPairedVersion is set, how about simplifying it to a Revision struct, not pointer. Then revisionPtr can be removed as well.

carolynvs · 2017-08-07T16:49:20Z

cmd/dep/godep_importer_test.go

@@ -395,3 +338,13 @@ func equalImports(a, b []godepPackage) bool {

 	return true
 }
+
+// projectRootPtr takes a ProjectRoot value and returns a pointer.
+func projectRootPtr(p gps.ProjectRoot) *gps.ProjectRoot {


I've laid out in the comments an alternative way to simplify the test cases and remove these helper functions. Let me know if they make sense.

I don't mean to hate on them, but if we can reorganize the tests to not need them, that would make me happy. 😀

@carolynvs

deferring to @carolynvs

carolynvs

Wahoo! Thank you for applying some cleansing fire to our tests. 🔥

sebdah requested review from carolynvs and sdboyer as code owners July 25, 2017 15:25

googlebot added the cla: yes label Jul 25, 2017

sebdah force-pushed the issue/824 branch 3 times, most recently from 419855f to 8f12cdb Compare July 26, 2017 01:02

sebdah changed the title ~~[WIP] Table based tests for importers~~ Table based tests for importers Jul 26, 2017

sdboyer suggested changes Jul 26, 2017

View reviewed changes

sdboyer previously requested changes Jul 28, 2017

View reviewed changes

darkowlzz reviewed Jul 29, 2017

View reviewed changes

carolynvs suggested changes Jul 31, 2017

View reviewed changes

carolynvs suggested changes Aug 7, 2017

View reviewed changes

sebdah force-pushed the issue/824 branch from c9a1a06 to 6c82b96 Compare August 9, 2017 18:13

sebdah added 12 commits August 9, 2017 20:42

Make the godep importer tests table based

bcc0b08

Make glide importer tests table based

8626836

fixup! Make the godep importer tests table based

7f3932a

fixup! Make glide importer tests table based

75c187e

fixup! Make the godep importer tests table based

c31aeda

fixup! Make glide importer tests table based

807f8cd

fixup! Make the godep importer tests table based

e03b18b

fixup! Make glide importer tests table based

7c46b99

fixup! Make the godep importer tests table based

cb7eb9a

fixup! Make the godep importer tests table based

cf94ba9

fixup! Make glide importer tests table based

cbc07aa

fixup! Make the godep importer tests table based

1df5ee5

sebdah force-pushed the issue/824 branch from faf4339 to 1df5ee5 Compare August 9, 2017 18:46

carolynvs approved these changes Aug 9, 2017

View reviewed changes

carolynvs merged commit 044c4ef into golang:master Aug 9, 2017

carolynvs mentioned this pull request Aug 11, 2017

Apply common validations to all import conversion unit tests #992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table based tests for importers #897

Table based tests for importers #897

sebdah commented Jul 25, 2017 •

edited

Loading

sdboyer left a comment

sdboyer Jul 26, 2017

sebdah commented Jul 26, 2017 via email

sebdah commented Jul 27, 2017

sdboyer left a comment

sdboyer Jul 28, 2017

sdboyer Jul 28, 2017

sebdah Jul 29, 2017

sdboyer Jul 30, 2017

sdboyer Jul 28, 2017

sebdah Jul 29, 2017

sdboyer Jul 28, 2017

sebdah commented Jul 29, 2017

darkowlzz Jul 29, 2017

carolynvs Jul 31, 2017 •

edited

Loading

sebdah Aug 3, 2017

sdboyer Aug 7, 2017 •

edited

Loading

carolynvs Aug 7, 2017

sebdah Aug 9, 2017

carolynvs left a comment

carolynvs Aug 7, 2017

carolynvs Aug 7, 2017

carolynvs Aug 7, 2017

carolynvs Aug 7, 2017

carolynvs left a comment

Table based tests for importers #897

Table based tests for importers #897

Conversation

sebdah commented Jul 25, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

To do

Which issue(s) does this PR fix?

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdah commented Jul 26, 2017 via email

sebdah commented Jul 27, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebdah commented Jul 29, 2017

Choose a reason for hiding this comment

carolynvs Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolynvs left a comment

Choose a reason for hiding this comment

sebdah commented Jul 25, 2017 •

edited

Loading

carolynvs Jul 31, 2017 •

edited

Loading

sdboyer Aug 7, 2017 •

edited

Loading