[Webkit-unassigned] [Bug 49122] webkitpy/tool/* unittests change cwd and don't clean up properly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49122





--- Comment #4 from Eric Seidel <eric at webkit.org>  2010-11-05 23:16:53 PST ---
(In reply to comment #0)
> In the course of merging and preparing to land the patch for bug 48280 , I stumbled into this.
> 
> various unit tests under webkitpy/tool/* change the current working directory and don't seem to change back when they're done. For example, download_unittest.test_apply_attachment creates a MockSCM() object, and then invokes a series of steps including steps/cleanworkingdirectory.py (which has the effect of changing to /private/tmp ).

Yeah, we have to be careful to use MockSCM().checkout_root and avoid using os.getcwd() as otherwise tests may or may not have manipulated the cwd.

> This by itself isn't that bad. 
> 
> However, the real scm checkout code uses the current working directory to figure out where the checkout root is, in default_scm():
> 
> def default_scm():
>     cwd = os.getcwd()
>     scm_system = detect_scm_system(cwd)
>     if not scm_system:
>         script_directory = os.path.abspath(sys.path[0])
>         scm_system = detect_scm_system(script_directory)
>         if scm_system:
>             log("The current directory (%s) is not a WebKit checkout, using %s" % (cwd, scm_system.checkout_root))
>         else:
>             error("FATAL: Failed to determine the SCM system for either %s or %s" % (cwd, script_directory))
>     return scm_system
> 
> The first part fails, so we fall back to os.path.abspath(sys.path[0]).
> 
> This by itself also isn't that bad. Normally, in python, sys.path[0] is defined to be the directory of the script that was invoked (i.e., test-webkitpy, so WebKitTools/Scripts) .
> 
> Sadly, in the monkeying with paths that we do now in test-webkit that seems to have something to do with making sure that QueueStatusServer is in our path, we call out to the WebKitTools/QueueStatusServer/__init__.py:fix_sys_path() which manipulates sys.path .

I *just* added this fix_sys_path code.  I should probably remove it and or fix it.  It seems to be the major problem here.

> Even that's not so bad. Sadly, that code also calls google_appengine.fix_sys_path(). And THAT code, at least in the version I happen to have installed on my machine (1.3.8), *prepends* EXTRA_PATHS to sys.paths, which breaks the assumption that sys.path[0] will be inside the tree.
> 
> So, if any test code then attempts to call the real scm.default_scm() code, it will fail. Which happens after the above unittests execute (and we've changed the directory), when we run commands/rebaseline_unittest, when that code constructs a layout_tests/port object, which calls the real scm code when I've merged in the changes in 48280.

Hmm.. seems slightly fishy that we use real scm code...

> A comedy of cascading errors, so to speak.

Agreed. :)

> There are several things that can/should be fixed here.
> 
> (1) The AppEngine code is probably wrong.
> 
> (2) the tool/commands unittests should not actually be changing directories.
> 
> (3) It may be bad to be relying only on sys.path[0] and cwd, since clearly both of those things can be outside the tree. The only other mechanism I can think of which should be safe would be to look at __file__ from inside scm.py . I think there are times when even that doesn't work right, because there was a reason I started using sys.path[0] instead of __file__ , but those reasons may not apply in this usage. I don't remember exactly what they were.

Yeah. :)  webkit-patch tries hard to make it so that you can have webkit-patch in your path, but use it with a different checkout.  As long as that use case doesn't beak, we can basically do whatever we want here. :)

> (4) Arguably the rebaseline_unittest command should be trying to get a test port, not a real port, and it should be passing in a MockConfig() object so that we don't use the real scm detection code. But there might need to be other stuff that changes in that test, I haven't really explored this yet.
> 
> I think (3) is safe, so I'll upload a patch for that.

I'd like to better understand the contract behind sys.path[0] before r+'ing this.  I should go read some python docs and see if I can figure out how we ended up using sys.path[0].  Maybe other things will break now as a result of this new AppEngine code?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.