568642 - enable `make mozmill` in mozilla-central to work in the usual way

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jeff Hammel]

In bug 516984, mozmill has been included in the mozilla-central tree for the `make package-tests` stage so that buildbot could run mozmill from the packages.  However, there is no convenience function (`make mozmill`) presented to developers and testers and mozmill is not runnable outside of when it is put in the `test-package-stage` directory.

A convenience function, `make mozmill`, should be provided to developers and testers that functions in a way parallel to the other test harnesses (e.g. `make xpcshell-tests`)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jeff, how is it bound to Mozmill 2.0 development?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is getting more important, now that developers also have to take care of Mozmill tests.

Comment 4

[Jeff Hammel]

It shouldn't be bound to mozmill 2.0 AFAICT. Flag removed

Comment 5

[Jeff Hammel]

Porting comments from duplicate bug:

If we expect developers to care about the Mozmill suite, we should provide them
with an easy way to run the tests.  Here's my proposal:

Given the assumption that objdir is your object directory built with
--enable-tests, this should be all you need to run the suite:

make -C /path/to/objdir mozmill

And this should also work for running a single test:

make -C /path/to/objdir mozmill TEST_PATH=path/to/test.js

Comment 6

[Jeff Hammel]

I have a basic patch up at:

http://k0s.org/mozilla/hg/mozilla-central-patches/file/tip/bug-568642

It doesn't do mozmill-restart test nor does it do the TEST_PATH logic, but its
not a horrible first stab.

This doesn't quite work, however, because automation-build.mk, included to find
the path to the binary on all OSes, quotes the path to the binary: 

http://mxr.mozilla.org/mozilla-central/source/build/automation-build.mk#10

I'll file a subsequent bug involving separating out the binary-finding logic
from the rest of the script

Comment 7

[Jeff Hammel]

Attachment: first round of `make mozmill`; does not yet work with TEST_PATH

this will run mozmill and mozmill-restart on mozilla-central;  contingent upon the patch in bug 614366

Comment 8

[Jeff Hammel]

So TEST-PATH will be hard, since there is both mozmill and mozmill-restart.  

My proposal:

- have a target, `make mozmill-all`; this will run all the tests both normal + restart and ignore TEST_PATH
- have a target, `make mozmill`, that will run the normal tests and respect TEST_PATH
- have a target `make mozmill-restart`, that will run the restart tests and respect TEST_PATH

What do people think?  It wouldn't be hard to do if this is what people want.  Also the above patch should be assessed to make sure its what we want to do

Comment 9

[(not currently active) Ted Mielczarek]

This sounds like a fine plan.

Comment 10

[(not currently active) Ted Mielczarek]

Although maybe "make mozmill" should just run both sets, since people might not know the distinction? It might be good if "make mozmill" ran whatever tinderbox runs.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #10)
> Although maybe "make mozmill" should just run both sets, since people might not
> know the distinction? It might be good if "make mozmill" ran whatever tinderbox
> runs.

+1

make mozmill-norestart and make mozmill-restart perhaps?

</bikeshedding>

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #8)
> So TEST-PATH will be hard, since there is both mozmill and mozmill-restart.  

Well, could make check the specified path and simply distinct which type of mozmill command has to be called, given if "restartTests" is in the path?

Comment 13

[Jeff Hammel]

I'm not in favor of magically looking for restartTests in TEST_PATH.  For one, its a lot of logic to put in a makefile and I'm inclined to avoid this as a temporary measure.  For another, it can be messed up, e.g. if I'm in restartTests and do `make mozmill TEST_PATH=.` or if you're trying restart tests in another staging directory.  And again, this can be solved correctly with manifests (bug 585106)

Comment 14

[(no longer active)]

FWIW, we _can_ have two test targets using the exact same harness but picking up the tests to run from different location.  The historical precedence for this are |make reftest| and |make crashtest|.

Comment 15

[Jeff Hammel]

FWIW, mozmill and mozmill-restart are not the same harness.  There is work on master to make them behave as such (deprecating the mozmill-restart entry point in favor of mozmill --restart), but the underlying logic is quite different and will remain so

Comment 16

[(no longer active)]

(In reply to comment #15)
> FWIW, mozmill and mozmill-restart are not the same harness.  There is work on
> master to make them behave as such (deprecating the mozmill-restart entry point
> in favor of mozmill --restart), but the underlying logic is quite different and
> will remain so

So, using two different targets makes even more sense, rigt?

Comment 17

[cmtalbert]

(In reply to comment #16)
> (In reply to comment #15)
> > FWIW, mozmill and mozmill-restart are not the same harness.  There is work on
> > master to make them behave as such (deprecating the mozmill-restart entry point
> > in favor of mozmill --restart), but the underlying logic is quite different and
> > will remain so
> 
> So, using two different targets makes even more sense, rigt?

Yes, two different targets make sense.  I also like the idea of make mozmill being whatever tinderbox is doing so that it's easier.  THe more arcanely named targets can be used for specific "restart only" or "no-restart-test" capabilities.

Comment 18

[Jeff Hammel]

Attachment: enable `make mozmill` in mozilla-central to work in the usual way

Comment 19

[cmtalbert]

Not working for me (on windows):
Traceback (most recent call last):
  File "./_tests/mozmill/installmozmill.py", line 136, in <module>
  File "./_tests/mozmill/installmozmill.py", line 129, in main
  File "./_tests/mozmill/installmozmill.py", line 63, in python_script_path
NameError: global name 'basename' is not defined
make: *** [_tests/mozmill] Error 1

And on mac it looks like we do not have the right binary type set.  I get "invalid CPU type" when it accesses the firefox in the object directory

Comment 20

[Jeff Hammel]

Attachment: enable `make mozmill` in mozilla-central to work in the usual way

This will hopefully fix the basename issue on windows.

Comment 21

[Jeff Hammel]

> And on mac it looks like we do not have the right binary type set.  I get
> "invalid CPU type" when it accesses the firefox in the object directory

This looks like a mozmill bug.  I thought we were calling arch under (whatever special set of circumstances) in mozrunner.  But...now I'm confused.  Does anyone know the status of this, or must we spelunk?

Comment 22

[Jeff Hammel]

(In reply to comment #21)
> > And on mac it looks like we do not have the right binary type set.  I get
> > "invalid CPU type" when it accesses the firefox in the object directory
> 
> This looks like a mozmill bug.  I thought we were calling arch under (whatever
> special set of circumstances) in mozrunner.  But...now I'm confused.  Does
> anyone know the status of this, or must we spelunk?

Ah, here's the code:

https://github.com/mozautomation/mozmill/blob/hotfix-1.5.2/mozrunner/mozrunner/__init__.py#L456

It doesn't seem to have made it to master.  I don't really understand mac well enough to know what the right set of conditions are here.

Comment 23

[Jeff Hammel]

Attachment: cleanup, install in development mode amongst other fixes

updated version of the patch;  fixes some problems.  still, its awful code

Comment 24

[cmtalbert]

This works on linux, but it still doesn't work on windows:
$ make mozmill
make -C ./testing/mozmill install PKG_STAGE=../../_tests
make[1]: Entering directory `/c/Users/ctalbert/projects/mc/obj-dbg-win32/testing
/mozmill'
c:/Users/ctalbert/projects/mc/obj-dbg-win32/config/nsinstall.exe -D ../../_tests
/mozmill
echo simplejson-2.1.1 mozrunner jsbridge mozmill  > ../../_tests/mozmill/PACKAGE
S
c:/Users/ctalbert/projects/mc/obj-dbg-win32/config/nsinstall.exe  "/c/Users/ctal
bert/projects/mc/testing/mozmill/simplejson-2.1.1"  "/c/Users/ctalbert/projects/
mc/testing/mozmill/mozrunner"  "/c/Users/ctalbert/projects/mc/testing/mozmill/js
bridge"  "/c/Users/ctalbert/projects/mc/testing/mozmill/mozmill" ../../_tests/mo
zmill/
nsinstall: c:\Users\ctalbert\projects\mc\testing\mozmill\simplejson-2.1.1 is a d
irectory
nsinstall: c:\Users\ctalbert\projects\mc\testing\mozmill\mozrunner is a director
y
nsinstall: c:\Users\ctalbert\projects\mc\testing\mozmill\jsbridge is a directory

nsinstall: c:\Users\ctalbert\projects\mc\testing\mozmill\mozmill is a directory
make[1]: *** [install] Error 3
make[1]: Leaving directory `/c/Users/ctalbert/projects/mc/obj-dbg-win32/testing/
mozmill'
make: *** [_tests/mozmill] Error 2

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsinstall on Windows doesn't recursively copy directories

Comment 26

[Jeff Hammel]

(In reply to comment #25)
> nsinstall on Windows doesn't recursively copy directories

Hmm, it probably should :(

I can hack around this....i left some discrimination of platform in the Makefile script, but its probably not robust (think WINCE, WINMO) and I am not sure if it can be made robust with the current system.

If we don't want to fix our build system, I'm inclined to move more logic to installmozmill.py (which I would rename, except buildbot currently depends on the existing name).

Comment 27

[Jeff Hammel]

Attachment: more fixes to try to get to work on windows

I haven't tested this on windows.  If this doesn't work or exhibits more fatal flaws, I'll probably flush out installmozmill.py to do even more stuff.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #26)
> (In reply to comment #25)
> > nsinstall on Windows doesn't recursively copy directories
> 
> Hmm, it probably should :(

Bug 590538.

I'd rather just finish the python version of nsinstall though and drop the binary version entirely.

Comment 29

[Jeff Hammel]

Attachment: proposed final patch

this works for me locally;  i'm not really sure about the structure, though i guess its no worse than most of what we already have

Comment 30

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

Comment on attachment 497526 [details] [diff] [review]
proposed final patch


>diff --git a/testing/mozmill/Makefile.in b/testing/mozmill/Makefile.in
>--- a/testing/mozmill/Makefile.in
>+++ b/testing/mozmill/Makefile.in
>@@ -59,24 +59,42 @@ include $(topsrcdir)/config/rules.mk
>-# Rules for staging the necessary harness bits for a test package
>-PKG_STAGE = $(DIST)/test-package-stage
>-
>-stage-package:
>+# assumes you are in objdir
>+mozmill-dir:
> 	$(NSINSTALL) -D $(PKG_STAGE)/mozmill
> 	echo $(TEST_HARNESS_PACKAGES) > $(PKG_STAGE)/mozmill/PACKAGES
>+
>+install: mozmill-dir
> 	(cd $(srcdir) && tar $(TAR_CREATE_FLAGS) - $(TEST_FILES)) | (cd $(PKG_STAGE)/mozmill && tar -xf -)
> 
>+# on WINNT, copy the tests; otherwise, assume you can symlink them
>+ifeq ($(OS_ARCH),WINNT)
>+install-develop: install
>+else
>+install-develop: mozmill-dir
>+	$(INSTALL) $(foreach f, $(TEST_HARNESS_PACKAGES), "$(srcdir)/$f") $(PKG_STAGE)/mozmill/
>+	(cd $(srcdir) && tar $(TAR_CREATE_FLAGS) - $(MOZMILL_EXTRAS)) | (cd $(PKG_STAGE)/mozmill && tar -xf -)
>+	$(INSTALL) $(topsrcdir)/testing/mozmill/tests $(PKG_STAGE)/mozmill
>+endif
>+
>+# Rules for staging the necessary harness bits for a test package
>+stage-package: PKG_STAGE = $(DIST)/test-package-stage
>+stage-package: install


From the comment and the code, it appears that Linux and OSX we use symlinks.  Here is where I could be confused as 
PKG_STAGE typically means the 'make package-stage' result in $(objdir)/dist/test-package/stage/...  



>diff --git a/testing/testsuite-targets.mk b/testing/testsuite-targets.mk
>--- a/testing/testsuite-targets.mk
>+++ b/testing/testsuite-targets.mk
>+# install and run the mozmill tests
>+$(DEPTH)/_tests/mozmill:
>+	$(MAKE) -C $(DEPTH)/testing/mozmill install-develop PKG_STAGE=../../_tests
>+	$(PYTHON) $(topsrcdir)/testing/mozmill/installmozmill.py --develop $(DEPTH)/_tests/mozmill
>+
>+MOZMILL_TEST_PATH = $(DEPTH)/_tests/mozmill/tests/firefox
>+mozmill: TEST_PATH?=$(MOZMILL_TEST_PATH)
>+mozmill: $(DEPTH)/_tests/mozmill
>+	bash $(DEPTH)/_tests/mozmill/mozmill.sh -t $(TEST_PATH) -b $(browser_path) --show-all
>+

are we reusing PKG_STAGE for mozmill and maybe cannibalising it for other uses?


These are the things that stood out at me.  I think it works as expected, but reading the patch is confusing and could
be with future people editing the same files.  For the rest of the patch, I am not as familiar and my lightweight overlooking
didn't throw up red flags.

Comment 31

[Jeff Hammel]

PKG_STAGE is a bit cannibalized with respect to intent.  I could introduce another variable, TARGET, if that will make things clearer. (I mostly didn't because its difficult for me to test on non-linux).  So many things in the makefiles are done completely differently from everything else, it is hard to pick up what is a good vs a bad vs an okay pattern

Comment 32

[cmtalbert]

Comment on attachment 497526 [details] [diff] [review]
proposed final patch

	
>diff --git a/testing/mozmill/installmozmill.py b/testing/mozmill/installmozmill.py
>--- a/testing/mozmill/installmozmill.py
>+++ b/testing/mozmill/installmozmill.py
>@@ -38,61 +38,121 @@
> # ***** END LICENSE BLOCK *****
> 
> """
> install mozmill and its dependencies
> """
> 
> import os
> import sys
>+from optparse import OptionParser
> from subprocess import call
> 
>+  # create a front end runner that is path-independent
>+  template = """#!/bin/bash
>+unset PYTHONHOME
>+%(PYTHON)s %(MOZMILL)s $@
>+"""
>+  variables = {'PYTHON': entry_point_path(destination, 'python') }
>+  for script in 'mozmill', 'mozmill-restart':
>+    path = os.path.join(destination, script + '.sh')
>+    f = file(path, 'w')
>+    variables['MOZMILL'] = python_script_path(destination, script)
>+    print >> f, template % variables
>+    f.close()
>+    if not is_windows():
>+      os.chmod(path, 0755)
I think this is a good idea but it won't work in a msys bash shell on windows because os.join will give you standard windows style paths of c:\foo\baz.  And when this script is interpretted by bash they will become c:foobaz which will fail.  You'll need to replace \ with \\ so that you have paths like: c:\\foo\\baz on windows platforms.  That should work.

Otherwise, the changes do look good.  r+ with that ^ issue fixed.  If you have a fix for that, then I will run it on windows for you.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #32)
> I think this is a good idea but it won't work in a msys bash shell on windows
> because os.join will give you standard windows style paths of c:\foo\baz.  And
> when this script is interpretted by bash they will become c:foobaz which will
> fail.  You'll need to replace \ with \\ so that you have paths like:
> c:\\foo\\baz on windows platforms.  That should work.

That's already done by join. It uses '\\' internally. At least on my box. Or do I miss something?

Comment 34

[Jeff Hammel]

The problem is, its writing a shell script.  Plus, what do you mean internally?  in python representation? or?

Comment 35

[Jeff Hammel]

> I think this is a good idea but it won't work in a msys bash shell on windows
> because os.join will give you standard windows style paths of c:\foo\baz.  And
> when this script is interpretted by bash they will become c:foobaz which will
> fail.  You'll need to replace \ with \\ so that you have paths like:
> c:\\foo\\baz on windows platforms.  That should work.

Do we only care about bash on windows?  Or do we care about msys too?

Comment 36

[cmtalbert]

Attachment: Unbitrotted patch

my unbitrotted version of the patch that I used to review and test.

Comment 37

[Jeff Hammel]

Attachment: proposed final patch

I haven't tested on windows yet, but maybe (?) this fixes the issue there. (It works on linux....i think it *should* work on windows in a bash environment, but i'm still updating my build environ to test).  I also added in a critical $(TEST_PATH) variable that I previously forgot from `make mozmill-restart`.

I didn't find this patch to be bitrotten.  Except for these changes, it should be of the same form as the previous.  What issues were you seeing when applying Clint? (If you can't recall, its probably not important).

Carrying over the r+ from :ctalbert

Comment 38

[cmtalbert]

Comment on attachment 499434 [details] [diff] [review]
proposed final patch

Using c:\users\ctalbert\projects\mc\testing\mozmill\jsbridge
Finished processing dependencies for mozmill==1.5.1rc2
bash ./_tests/mozmill/mozmill.sh -t ./_tests/mozmill/tests/firefox -b ./dist/bin
/firefox.exe --show-all
./_tests/mozmill/mozmill.sh: line 3: c:Usersctalbertprojectsmcobj-dbg-win32_test
smozmillScriptspython.exe: command not found
make: *** [mozmill] Error 127

ctalbert@CTALBERT-LAPTOP ~/projects/mc/obj-dbg-win32
$ cat _tests/mozmill/mozmill.sh
#!/bin/bash
unset PYTHONHOME
c:\Users\ctalbert\projects\mc\obj-dbg-win32\_tests\mozmill\Scripts\python.exe c:
\Users\ctalbert\projects\mc\obj-dbg-win32\_tests\mozmill\Scripts\mozmill-script.
py $@

I don't see anything in this patch that fixes the problem on windows and as you can see from the above output of make mozmill, this doesn't work.  You need to detect that you're on windows when you're building mozmill.sh and use paths of the style c:\\foo\\baz.  Minused on feedback as this patch is not ready to go.

Comment 39

[cmtalbert]

Also, did you test this on mac?

Comment 40

[Jeff Hammel]

Attachment: proposed final patch

Oops, attached the wrong patch.  This one uses the esc() function to escape to strings, which as best as I can works on windows in bash environment (only superficially tested).  Has not been tested on mac, but should work with any bash shell, I think

Comment 41

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

You should use '$(SHELL)' instead of 'bash' in your makefiles.

Comment 42

[Jeff Hammel]

Attachment: proposed final patch

use `$(SHELL)` instead of `bash` in makefiles,

Comment 43

[cmtalbert]

Comment on attachment 501070 [details] [diff] [review]
proposed final patch

Looks good.  Also tested on windows and it passed with flying colors.

Comment 44

[cmtalbert]

Landed http://hg.mozilla.org/mozilla-central/rev/7a5beb46743f

Comment 45

[Jeff Hammel]

There is a bug whereby `make mozmill-all` will err out.  This will only occur with bad versions of make -- sadly, I can't find the bug -- and only with mozmill-all; `make mozmill` and `make mozmill-restart` are fine.

Running this (Ubuntu 10.10, Lenovo T510 Thinkpad), I get the following:

make mozmill TEST_PATH=./_tests/mozmill/tests/firefox
make[1]: Entering directory `/home/jhammel/mozilla/src/obj-browser'
/bin/sh ./_tests/mozmill/mozmill.sh -t 8�Ȭt-2 -b ./dist/bin/firefox-bin --show-all

Even though TEST_PATH is correctly defined, doing the subshell make turns it into garbage.  I doubt there is a workaround except upgrading or downgrading make

Comment 46

[(not currently active) Ted Mielczarek]

See bug 610290 comment 11.