Moving int tests to fixture repositories #265
Conversation
|
@sdboyer - After adding a few more integration test cases to init, I see a behavior I don't understand. When referencing repos that have not yet been retrieved, there is one case where the unretrieved repo is left off the manifest, but another where its "master" branch is added. (Look at the resulting golden files to see the output.) Is this correct? If not, I can open an issue. |
|
@sdboyer - no feedback here, so I just finished off the conversion while I have time. If this is not the path you were looking for, please let me know and I'll close the request. CI passing is still spotty on my machine, with 1 time in 4 failing due to the remove test / gps interaction. The suite always passes when skipping the remove test. |
|
@tro3 sorry, all the spare time i've had for the last week has been devoted to trying to get the roadmap together :/ i'll review as soon as i possibly can - thanks for keeping at it! |
tro3
changed the title
|
No worries and no hurry. As long as we're not in "black hole" mode. :-) |
|
for sure not in black hole mode :) until i do get to it, it seems like travis is complaining about a gofmt issue - have a look at that? |
|
Oh - I must have misinterpreted the failure. (I saw the password requests and thought it was the remove test thing again.) I'll take a look. Thx.
|
Could you point me more specifically at a place to look for this? |
|
testdata/init/case2/manifest.golden.json vs case3 manifest. The test cases that generate these are in init_test, lines 68-95
|
There was a problem hiding this comment.
I really like how this is starting to become more structured - the table-based tests, the layout of the files on disk. Much more systematic, which is awesome.
I'm thinking I might like to see this structured even a little further. For example, instead of explicitly having each table-based test entry declare the name of its golden manifest and lock file, just have that be a hardcoded decision, reflected by filesystem layout and naming. Similarly, don't declare rules for which go files to copy over - just have general logic that infers what to do based on the arrangement of the filesystem.
Does that make sense, and seem sane? My goal is to drive the locus of control for test behavior into a single location (disk vs. logic in the test) as much as possible. It seems like we can cover all cases that way, at least for now; if not, I'd be OK with covering almost all cases that way, then having a couple others that we have to manage more manually.
If we go that route, then let's also add a README to the cmd/dep/testdata directory that explains the rules behind the layout. Or one to each subdir, if appropriate.
cmd/dep/ensure_test.go
Outdated
| } | ||
|
|
||
| for _, testCase := range tests { | ||
| runTest(t, testCase) |
There was a problem hiding this comment.
If you're gonna set it up like this (which is great), it's probably worth just executing them as subtests.
cmd/dep/init_test.go
Outdated
| defer h.Cleanup() | ||
| runTest := func(t *testing.T, testCase initTestCase) { | ||
| test.NeedsExternalNetwork(t) | ||
| test.NeedsGit(t) |
There was a problem hiding this comment.
It's enough to just do these checks once, outside the loop.
cmd/dep/init_test.go
Outdated
| // Build a fake consumer of these packages. | ||
| root := "src/github.com/golang/notexist" | ||
| for src, dest := range testCase.sourceFiles { | ||
| h.TempCopy(root+"/"+dest, testCase.dataRoot+"/"+src) |
There was a problem hiding this comment.
Instead of infixing the "/" here, use filepath.Join() here - it'll do the cross-platform thing for you.
There was a problem hiding this comment.
Yeah, I debated that move. Clearly not best practice the way I did it yet I am running on a PC and it is handling the "/" well.
I'll fix them all. I just wish the name for that function wasn't so long - hurts readability IMO
cmd/dep/init_test.go
Outdated
| // vendor should have been created & populated | ||
| h.MustExist(h.Path(root + "/vendor/github.com/Sirupsen/logrus")) | ||
| for _, testCase := range tests { | ||
| runTest(t, testCase) |
There was a problem hiding this comment.
Same deal as above - let's use subtests here.
cmd/dep/init_test.go
Outdated
| h.Cd(h.Path(root)) | ||
| h.Run("init") | ||
|
|
||
| wantManifest := h.GetTestFileString(testCase.dataRoot + "/" + testCase.goldenManifest) |
cmd/dep/init_test.go
Outdated
| gotManifest := h.ReadManifest() | ||
| if wantManifest != gotManifest { | ||
| if *test.UpdateGolden { | ||
| if err := h.WriteTestFile(testCase.dataRoot+"/"+testCase.goldenManifest, gotManifest); err != nil { |
There was a problem hiding this comment.
(i'm not gonna explicitly note any more of these, i think you have the picture 😄 )
| func main() { | ||
| err := nil | ||
| if err != nil { | ||
| deptestdos.diMeLo(bar.Qux) |
There was a problem hiding this comment.
While we don't currently do any actual code-level analysis (beyond import statements) during solving, it's possible we might add it in the future. If/when we do, these tests may start erroneously failing if they refer to nonexistent members in the deptest packages.
To guard against that possibility, let's make sure these input fixtures are valid uses of those packages - make sure you reference them somehow (so that it's not an unused import), but also make sure to only use types that are actually present.
Boring, I know 😈
| "dependencies": { | ||
| "github.com/sdboyer/deptest": { | ||
| "version": ">=0.8.0, <1.0.0" | ||
| } |
There was a problem hiding this comment.
Yeah, I don't know what's going on here. Looking at how this is set up, it seems like this manifest really should have an entry for both deptest and deptestdos. Let's make sure we figure out why this is happening before merging - perhaps we've found a bug.
|
...actually, I think maybe we can delay further abstracting the harness until a subsequent PR. |
|
I do like the abstraction idea. Let's finish off the above review changes and merge, and I'll submit an "abstract test harness" issue where we can discuss the structure there in parallel, before I dive into the implementation. |
|
I think we're looking pretty good here. Anything else you want to add to it? |
|
For now, no. I'm working on the abstract test harness thing. First pass is running, but needs a little more before I submit the WIP PR.
|
|
In case I was unclear before, this one is okay to merge and would in fact be convenient. My harness branch is off of master, so merging here would allow me to more easily fold in these tests.
|
|
got it - was just trying to make appveyor go green. really need to fix that :( |
@sdboyer, looking for feedback on this path for dealing with #255. Have moved the pointers to the fixed repos, but also refactored to allow for new test cases to be added more easily. Pls le me know what you think of this path.