Integration tests failing?

[tro3]

In trying to move the hard-wired files to a separate directory, I found I could not get the integration tests to pass, even on the current master. Before I go digging, is anyone else having this problem? (Trying to rule out my setup.) I am running go test github.com/golang/dep/cmd/dep on go1.7.4 windows/amd64.

FWIW, I notice appveyor produced the same error on my PR - it can't remove a temp file because it is in use by another process. Another data point is that on my PC, at least, the process ends thinking it is still running git (asking me for a password) which had clearly been launched during the test. I'd suspect that as being the "other process".

[peterbourgon]

@jessfraz any ideas here?

[jessfraz]

Has the file not been closed?

…

On Thu, Feb 2, 2017, 17:31 Peter Bourgon ***@***.***> wrote: @jessfraz <https://github.com/jessfraz> any ideas here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#214 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYNbBo4XaZV_K2n4_Dxl-lqlTGaELgmks5rYoN7gaJpZM4L0B5w> .

[tro3]

FWIW, I have it nailed down to specifically the RemoveTest, lines 90 and/or 114. (Each launch a process/thread that does not terminate, resulting in the "still in use" deletion error.) Both try to "remove" github.com/not/used. (Note: line 82 go remove -unused runs fine, even with github.com/not/used present.) In remove.go, it appears to be line 181 (s.Solve()) that launches the non-terminating processes, but I can't yet confirm it. Apologies I don't have it nailed down yet, but I am a little light on time today and only just recently learning Go paradigms.

[sdboyer]

The s.Solve() call could initiate some processes that don't complete. Improved process management is one of my ongoing goals in gps...though there is handling in place that should prevent the test from ending before all subprocesses have terminated.

What's path to the tempfile that it's complaining about not being able to remove?

@mattn's already been great in help tracking down some of these issues, and I had someone get me access to a windows box recently so that I could work on it more directly, too. Hopefully we can get these all hammered out soon :/

[tro3]

The "file in use" is always (tempdir)\src\github.com\golang\notexist. The two Runs that cause the issue don't actually reference this directory, though. The first is remove github.com/not/used and the other is remove -force github.com/pkg/errors github.com/not/used. I'll dig some more today.

[mattn]

Yes, seems to be running multiple tests. Sometimes running git on the directory when the error occured.

[tro3]

Okay, I've tracked it down. During the TestRemove test in gps/deduce.go, line 558, maybeSource.try is being called inside a closure with an internal go routine:

srcfut := func(mb maybeSource) partialSourceFuture {
	return func(cachedir string, an ProjectAnalyzer) sourceFuture {
		var src source
		var ident string
		var err error

		c := make(chan struct{}, 1)
		go func() {
			defer close(c)
			src, ident, err = mb.try(cachedir, an)
		}()

		return func() (source, string, error) {
			<-c
			return src, ident, err
		}
	}
}

This leads to gps/vcs_source.go line 147 in doListVersions() calling an external "git ls-remote repoName" command. But in the remove test, the repoName is github.com/not/exist. On Windows with no RSA token set, this process hangs asking for a GitHub password. The hanging process ties up the directory, so cleanup() can't clean up and we get the failure.

Not sure what to do about it, but that git ls-remote command in doListVersions() during TestRemove (only) is definitely what's killing the integration tests on my machine, anyway.

[sdboyer]

Great sleuthing @tro3! (side note, I'm curious what you did to track that down

I'm guessing it's not just on Windows where we have an underlying problem with calls hanging on a password request. The only difference on *nix-y platforms is that such hanging calls don't appear to cause the same issues with directory handles, so it doesn't result in test failures.

The more general solution to safety in this realm is probably sdboyer/gps#159, or at least sdboyer/gps#84, but there's still a bit of work between here and there. It's also...annoying, as I thought that sdboyer/gps#141 would have taken another step to address this. (Clearly not on Windows, but it's the same general problem)

Meanwhile, I'm going to see if just changing around the upstream path of the nonexistent repo can help us out.

[tro3]

I set up a more direct test case, targeting removeCommand with proper setup instead of the build-and-run pattern. Then I was able to get to it through a combo of debugger and good old-fashioned test code insertion. Glad it paid off.

[sdboyer]

Underlyign issue is worked out