Closed Bug 912243 Opened 11 years ago Closed 10 years ago

Mochitest shouldnt chdir in __init__

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla31

People

(Reporter: k0scist, Assigned: vaibhav1994)

References

Details

(Whiteboard: [good first bug][mentor=jmaher][lang=python])

Attachments

(1 file, 10 obsolete files)

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#177

from https://bugzilla.mozilla.org/show_bug.cgi?id=746243#c35 :

> > - MochiTest.__init__ :
> >     os.chdir(SCRIPT_DIR) ? why?

> Historical
> reasons, I suspect. We could almost certainly fix that without
> breaking things.
Whiteboard: [good first bug][mentor=jmaher][lang=python]
Assignee: nobody → vaibhavmagarwal
Attached patch runtests.patch (obsolete) — Splinter Review
Attachment #8385457 - Flags: review?(jmaher)
Comment on attachment 8385457 [details] [diff] [review]
runtests.patch

Review of attachment 8385457 [details] [diff] [review]:
-----------------------------------------------------------------

this fails on osx and linux.  What happens is ssltunnel fails to start, probably because we depend on the current directory.
Attachment #8385457 - Flags: review?(jmaher) → review-
Attached patch chdir.patch (obsolete) — Splinter Review
This fixes the issue on my computer.
Attachment #8385457 - Attachment is obsolete: true
Attachment #8390874 - Flags: review?(jmaher)
Comment on attachment 8390874 [details] [diff] [review]
chdir.patch

Review of attachment 8390874 [details] [diff] [review]:
-----------------------------------------------------------------

we have some more work to do related to pywebsocket and the path needed for importing related libraries.
Attachment #8390874 - Flags: review?(jmaher) → review-
so looking into this further, we have to modify:
1) launching mochitestserver (xpcshell + httpd.js) as the paths are relative
2) modify python websocket server as it depends on current directory for importing tools needed
3) modify a bunch of failing tests (not sure of the score here).  For example:
http://dxr.mozilla.org/mozilla-central/source/dom/datastore/tests/file_app.sjs#1

I suspect we would need to modify at least 20 files based on the results here:
https://tbpl.mozilla.org/?tree=Try&rev=82a5beed8be8

ted, do you think this is worth the effort?  I know it is ideal that we have less dependencies on changing directories for code reuse and modularization.
Flags: needinfo?(ted)
I think we could fix all of those by simply launching the subprocesses with the cwd set appropriately. Is there something more complicated that I'm not seeing?
Flags: needinfo?(ted)
Attached patch init.patch (obsolete) — Splinter Review
Thanks Ted for the valuable insight!
Attachment #8390874 - Attachment is obsolete: true
Attachment #8397946 - Flags: review?(jmaher)
Comment on attachment 8397946 [details] [diff] [review]
init.patch

Review of attachment 8397946 [details] [diff] [review]:
-----------------------------------------------------------------

lets run this on try, please address the two nits below.

::: testing/mochitest/runtests.py
@@ +158,5 @@
>        env["PATH"] = env["PATH"] + ";" + str(self._xrePath)
>  
>      args = ["-g", self._xrePath,
>              "-v", "170",
> +            "-f", os.path.join(self._httpdPath, "httpd.js"),

why did you remove the /

@@ +163,4 @@
>              "-e", """const _PROFILE_PATH = '%(profile)s'; const _SERVER_PORT = '%(port)s'; const _SERVER_ADDR = '%(server)s'; const _TEST_PREFIX = %(testPrefix)s; const _DISPLAY_RESULTS = %(displayResults)s;""" %
>                     {"profile" : self._profileDir.replace('\\', '\\\\'), "port" : self.httpPort, "server" : self.webServer,
>                      "testPrefix" : self.testPrefix, "displayResults" : str(not self._closeWhenDone).lower() },
> +            "-f", os.path.join(SCRIPT_DIR,"server.js")]

please add a space between SCRIPT_DIR and "server.js"
Attachment #8397946 - Flags: review?(jmaher) → review+
I misread the patch in my initial review, the / being removed was due to using os.path.join.
Attached patch init.patch (obsolete) — Splinter Review
updated the small nit.
Attachment #8397946 - Attachment is obsolete: true
Attachment #8397983 - Flags: review?(jmaher)
Comment on attachment 8397983 [details] [diff] [review]
init.patch

Review of attachment 8397983 [details] [diff] [review]:
-----------------------------------------------------------------

while this patch is good, it doesn't solve the content/media tests or chrome/browser-chrome tests.
Attachment #8397983 - Flags: review?(jmaher) → review-
Attached patch init.patch (obsolete) — Splinter Review
This patch fixes the issues that are currently seen on try server.
Attachment #8397983 - Attachment is obsolete: true
Attachment #8399576 - Flags: review?(jmaher)
Comment on attachment 8399576 [details] [diff] [review]
init.patch

Review of attachment 8399576 [details] [diff] [review]:
-----------------------------------------------------------------

failing on try server:
https://tbpl.mozilla.org/?tree=Try&rev=b001a3c5ff90

3 mochitest-chrome tests, please look at the tests and what resources they access- I bet there is something doing file IO.

As for browser-chrome, you to try running this outside of the mach command, I suspect we will see the error then.
Attachment #8399576 - Flags: review?(jmaher) → review-
Attached patch init.patch (obsolete) — Splinter Review
Attachment #8399576 - Attachment is obsolete: true
Attachment #8401452 - Flags: review?(jmaher)
Comment on attachment 8401452 [details] [diff] [review]
init.patch

Review of attachment 8401452 [details] [diff] [review]:
-----------------------------------------------------------------

we still need android fixed, make sure all path related calls in mochitest_options.py have a join with script_dir.

::: testing/mochitest/mochitest_options.py
@@ +506,5 @@
>          # code should never be executed in local runs because the build system
>          # should always set the flag that populates this variable. If buildbot ever
>          # passes this argument, this code can be deleted.
>          if options.testingModulesDir is None:
> +            possible = os.path.join(os.getcwd(), 'modules')

please ensure you try the original value and if that doesn't work, use your new value here.
Attachment #8401452 - Flags: review?(jmaher) → review-
Attached patch init.patch (obsolete) — Splinter Review
Attachment #8401452 - Attachment is obsolete: true
Attachment #8401498 - Flags: review?(jmaher)
Comment on attachment 8401498 [details] [diff] [review]
init.patch

Review of attachment 8401498 [details] [diff] [review]:
-----------------------------------------------------------------

this is failing on try server.
Attachment #8401498 - Flags: review?(jmaher) → review-
Attached patch init.patch (obsolete) — Splinter Review
Attachment #8401498 - Attachment is obsolete: true
Comment on attachment 8401843 [details] [diff] [review]
init.patch

Review of attachment 8401843 [details] [diff] [review]:
-----------------------------------------------------------------

was green on try and landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d459cc1b5a94
Attachment #8401843 - Flags: review+
backed out for rc1/rc2 failures on android :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c107142a477

here is the try run:
https://tbpl.mozilla.org/?tree=Try&rev=8a3edb08ce27

I did -u mochitests, but I guess I didn't do robocop as well :(
here is a log from an rc1 failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37363921&tree=Mozilla-Inbound&full=1

and the useful snippet:
Traceback (most recent call last):
  File "mochitest/runtestsremote.py", line 757, in <module>
    main()
  File "mochitest/runtestsremote.py", line 633, in main
    dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
  File "/builds/tegra-200/test/build/tests/mochitest/devicemanagerSUT.py", line 353, in pushFile
    raise DMError("DeviceManager: Error reading file to push")
devicemanager.DMError: DeviceManager: Error reading file to push
https://tbpl.mozilla.org/?tree=Try&rev=8a3edb08ce27&showall=1 - the still-hidden mochitest-e10s runs weren't happy either.
Attached patch init.patch (obsolete) — Splinter Review
Attachment #8401843 - Attachment is obsolete: true
Attached patch init.patch (obsolete) — Splinter Review
This patch passes on try.
Attachment #8406742 - Attachment is obsolete: true
Attachment #8407505 - Flags: review?(jmaher)
Comment on attachment 8407505 [details] [diff] [review]
init.patch

Review of attachment 8407505 [details] [diff] [review]:
-----------------------------------------------------------------

I just have one small nit, otherwise this looks great.  Please upload a patch with the added comment and carryover the r+

::: testing/mochitest/runtests.py
@@ +440,5 @@
>      manifest = None
>  
>      testRoot = self.getTestRoot(options)
> +    testdir = SCRIPT_DIR.split(os.getcwd())[-1]
> +    testdir = testdir.strip(os.sep)

please add a comment as to what testdir is and why it is needed.
Attachment #8407505 - Flags: review?(jmaher) → review+
Attached patch init.patchSplinter Review
Attachment #8407505 - Attachment is obsolete: true
Attachment #8407521 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/03e9ce5758cb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
I'm having problems running "make mochitest-robocop" now...possibly related to this change?

mozpad:~/objdirs/droid$ make mochitest-robocop
Error deleting /data/anr/traces.txt
Mochi-Remote INFO | Device info: {}
Mochi-Remote INFO | Test root: /mnt/sdcard/tests
Mochi-Remote INFO | Android sdk version '10'; will use this to filter manifests
Mochi-Remote INFO | TEST-INFO | skipping testAboutPage | skip-if: android_version == "10"
MochitestServer : launching [u'/home/gbrown/objdirs/firefox/dist/bin/xpcshell', '-g', '/home/gbrown/objdirs/firefox/dist/bin', '-v', '170', '-f', '/home/gbrown/objdirs/droid/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmp7x39Uw'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '192.168.0.82'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/gbrown/objdirs/droid/_tests/testing/mochitest/server.js']
runtests.py | Server pid: 27735
TEST-UNEXPECTED-FAIL | runtests.py | Timed out while waiting for server startup.
gbrown, are any of these paths incorrect?  possible: /home/gbrown/objdirs/droid/httpd.js ?
Yes, that is wrong!

If I backout 03e9ce5758cb, mochitest-robocop runs fine and uses:

MochitestServer : launching [u'/home/gbrown/objdirs/firefox/dist/bin/xpcshell', '-g', '/home/gbrown/objdirs/firefox/dist/bin', '-v', '170', '-f', '/home/gbrown/objdirs/droid/_tests/testing/mochitest/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpp08sLT'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '192.168.0.82'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', './server.js']
Depends on: 997991
ah, that fixed it for me :-D
You need to log in before you can comment on or make changes to this bug.