912243 - Mochitest shouldnt chdir in __init__

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Jeff Hammel]

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.

Comment 1

[Vaibhav (:vaibhav1994)]

Attachment: runtests.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 3

[Vaibhav (:vaibhav1994)]

Attachment: chdir.patch

This fixes the issue on my computer.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 6

[(not currently active) Ted Mielczarek]

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?

Comment 7

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Thanks Ted for the valuable insight!

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

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"

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I misread the patch in my initial review, the / being removed was due to using os.path.join.

Comment 10

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

updated the small nit.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 12

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

This patch fixes the issues that are currently seen on try server.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 14

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 16

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8401498 [details] [diff] [review]
init.patch

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

this is failing on try server.

Comment 18

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 22

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/?tree=Try&rev=8a3edb08ce27&showall=1 - the still-hidden mochitest-e10s runs weren't happy either.

Comment 23

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Comment 24

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

This patch passes on try.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 26

[Vaibhav (:vaibhav1994)]

Attachment: init.patch

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e9ce5758cb

Thanks for the patch!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/03e9ce5758cb

Comment 29

[Geoff Brown [:gbrown]]

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.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

gbrown, are any of these paths incorrect?  possible: /home/gbrown/objdirs/droid/httpd.js ?

Comment 31

[Geoff Brown [:gbrown]]

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']

Comment 32

[Mark Capella [:capella]]

ah, that fixed it for me :-D