New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel capability for integration tests #378

Merged
merged 10 commits into from Apr 16, 2017

Conversation

Projects
None yet
3 participants
@tro3
Copy link
Contributor

tro3 commented Apr 14, 2017

Going after #362.

Adds parallel capability by removing all "cd" commands from the test, running the entire test from one working directory. The directory creation and process-launching functions from TestHelper are not used, with constant-working-directory versions moved into TestProject. (Some dependency on TestHelper remains, and one could make an aurgument that this should be cleaned up.)

@googlebot googlebot added the cla: yes label Apr 14, 2017

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

Hmm - Travis is giving errors, where my machine shows all passes. Makes me wonder if there is a platform dependence to Cmd.Dir, or other exec dependence in the way. If so, the architecture to get to parallel capability is not robust. I'll dig, but we may have to pull the plug on this one.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

Well, the Linux builds are only failing remove/specific/case2, and it might be the earlier issue that gps fixes. But the Mac build is a train wreck. Unless someone sees an obvious issue with the code, I'll have to pull the PR, as I have no way to debug on a mac.

In either case, maybe worth revisiting when the new gps shows up.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

@sdboyer - when do you plan to merge the new gps into dep? I will retry then to see if the platform dependency drops.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 14, 2017

@tro3 I'll probably do another merge this weekend. Once I do, I'll loop back here and look at the mac failures as well.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

@sdboyer - I may be on to a subtle issue on my end, so the jury is still out. Let's each work our angles and meet on the other side. :-)

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

Okay - the Linux failures were my fault, and those are now fixed. So PC and Linux are now clean. The Mac failures I cannot yet speak to, but will now try to find a pattern in the failing tests.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

Well, the pattern on Macs is pretty simple - dep (or git?) is not running. The only test that passes is ensure/update/case2, which has the unique distinction of having exactly equal before and after states and no vendor directories. Perhaps an issue with the Cmd.Dir property on the Mac implementation? Not much else has changed from the current master.

Hope this helps, but we're likely at the end of what I can do.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 14, 2017

BTW, when the new gps arrives, we need to lose integration_test.go:28-33. My availability is about to drop off on Monday, so I wanted to get that note in. FWIW, the remove tests currently pass on my PC, but I don't know how intermittently.

@sdboyer sdboyer referenced this pull request Apr 15, 2017

Merged

Update gps to v0.16.0 #382

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 15, 2017

@tro3 new gps is in - any chance you have time to poke at this a bit more before you drop off on Monday? 🙏 🙏 🙏

(also, we'll miss you!)

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 16, 2017

Okay - as one would expect, the Linux and PC platforms work in the new GPS, but the Mac version of the test is DOA. If you have any suggestions as to what the Mac issue might be, I am happy to try them out. In the interim, I'll submit a separate "enable the remove tests on Windows" PR just to make sure that gets in.

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 16, 2017

Oh - and the parallel tests now work on PC. So I've uncommented to see if they are running on Linux as well. Still doubt Mac will fly.

tro3 added some commits Apr 16, 2017

@tro3 tro3 force-pushed the tro3:parallel_tests branch 2 times, most recently from 8d8a8c6 to 1050f75 Apr 16, 2017

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 16, 2017

This PR needs help from someone developing on a Mac.

The architecture was simply to move from a "cd to projectDir and run command" to a "remain in test workingDir and launch command shell in projectDir" methodology. The "cd" operation was leaking across threads, preventing parallelism.

Something about the above does not work on a Mac. PC and Linux are ok and test times do indeed drop in half, but the Mac tests die with exit status 1 in every test. I've tried removing the trailing slash in DoRun (integration_testproj.go:151), but that did not fix the Max case and actually broke an early Linux case (which shows the platform sensitivity of the "launch in another directory" plan).

If someone with a Mac can dig deeper, I'm happy to incorporate any changes and help get this off the ground.

new.h.Setenv("GOPATH", new.h.Path("."))
new.h.Cd(new.Path(ProjectRoot))
if runtime.GOOS == "darwin" {
new.Setenv("GOPATH", filepath.Join("/private", new.tempdir))

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

OHHHH THE STUPID /private LEADER

Yeah, that's one of those things that bites me, I curse at my mac for a while, then I promptly forget about. Sorry - that must've been a giant pain to track down through runs against a slow CI system 😦

I think we may get a bit more portability if you first check that the first element of new.tempdir is /tmp. Other than that, though, this strikes me as a sane way of compensating for this problem, at least for now.

This comment has been minimized.

@tro3

tro3 Apr 16, 2017

Contributor

Oh, this is a known thing? My first time running across it. The /tmp is dicey, as they are not /tmp in Windows.

This comment has been minimized.

@sdboyer

sdboyer Apr 16, 2017

Member

Yes, it's a thing - idk why, but in Apple's (bsd's?) infinite wisdom, they decided it's a good idea for /var, /etc, and /tmp to be symlinks to that respective name under a /private dir. Maybe it's a permissioning thing.

So, right - I meant, only do that extra check if we're on darwin.

This comment has been minimized.

@tro3

tro3 Apr 16, 2017

Contributor

Ok, will add along with documentation

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 16, 2017

Success!!

Okay, on Travis with the darwin mac platform, commands are run in a dir structure that adds "/private" in front. So all tests run with a remote dir launch were dying because they did not appear to be in a GOPATH. I've added a patch that adds "/private" to the environment GOPATH for darwin tests. I will now add documentation in the code.

Man, that is not the way to debug - created a new branch, limited the Travis tests to darwin, and add test logging and tweaks to read the output through the website. Not recommended. :-)

@tro3

This comment has been minimized.

Copy link
Contributor

tro3 commented Apr 16, 2017

@sdboyer ready for review. Will try to turn around any feedback tonight.

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Apr 16, 2017

Looks good! Thanks for sticking with this one!

@sdboyer sdboyer merged commit fe88577 into golang:master Apr 16, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tro3 tro3 deleted the tro3:parallel_tests branch Apr 16, 2017

ibrasho pushed a commit to ibrasho-forks/dep that referenced this pull request May 10, 2017

Merge pull request golang#378 from tro3/parallel_tests
Parallel capability for integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment