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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 5 20:33:06 PDT 2010


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

           Summary: webkitpy/tool/* unittests change cwd and don't clean
                    up properly
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Mac OS X 10.5
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Tools / Tests
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: dpranke at chromium.org
                CC: eric at webkit.org, abarth at webkit.org


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 ).

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 .

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.

A comedy of cascading errors, so to speak.

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.

(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.

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



More information about the webkit-unassigned mailing list