Closed Bug 1335873 Opened 7 years ago Closed 7 years ago

Convert marionette-harness tests to standard ./mach python-test unit tests

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(4 files)

Bug 1335873 - Ignore MozconfigFindException when looking for a mozinfo.json,
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1335873 - Convert marionette harness unittests to standard python unittests,
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1335873 - Add marionette-harness python tests to root moz.build file,
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1335873 - Check for .pyc's existence before deleting it in mach_bootstrap import hook,
59 bytes, text/x-review-board-request
glandium
: review+
Details 
Bug 1320194 and bug 1003417 are nearing completion. Once those are done, it means we'll have an easy mechanism of running python unittests in a standalone taskcluster task.

Once those are both done, we'll be able to convert the marionette harness tests to this format. Along the way we'll be able to clean up a bunch of code in mozharness, taskcluster and the python-test mach command.
Why has there been made the decision to drop pytest? Can you reference a discussion for that? I would be interested in, and Maja presumable too.
Sorry.. that is a terribly misleading title, my fault.

I meant convert them to standard python tests on the mozilla side of things, so using moz.build to collect manifests and using the taskcluster 'job' kind instead of mozharness and a standalone marionette-harness kind.

In practical terms, the only change from a user perspective should be that they will run:
./mach python-test testing/marionette

instead of:
./mach python-test --path-only testing/marionette/harness/marionette_harness/tests/harness_unit

Also, running ./mach python-test without arguments, would now include marionette harness tests in the full suite. We would continue to use pytest (mach python-test supports using both pytest or unittest).
Summary: Convert marionette-harness tests to a standard 'python unittests' → Convert marionette-harness tests to a standard ./mach python-test unit tests
For the record, I'd love if we could make pytest the standard and have all our python unittests using that.
Summary: Convert marionette-harness tests to a standard ./mach python-test unit tests → Convert marionette-harness tests to standard ./mach python-test unit tests
Thanks ahal, that sounds great!

On the topic of pytest, I’ve found they lend themselves well to writing the functional interaction kind of tests you write with Marionette.  We are using pytest already for the harness tests and for the WPT WebDriver tests, and it would be great if we could do the same for the Marionette “unit” tests.  This might be a good intern project.
There's the patch I was envisioning, and here's a try run with it in action:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6244df5a950b81b94fd41148e089ee87f712671d&selectedJob=73700466

You'll notice there are a bunch of failures in the harness tests, this is because there is no objdir in this new task. I see the same failures when running the following locally on a vanilla mozilla-central checkout:

$ ./mach clobber
$ ./mach python-test --path-only testing/marionette/harness/marionette_harness/tests/harness_unit

This means the marionette-harness tests require an objdir to run, I have two follow-up questions:

1) Do these tests legitimately require an objdir? Or is this a bug that can be fixed?
2) How does the mozharness based automation work around these failures? Do we have an objdir in those tasks?
This ended up being a bug in mozinfo easily solved by catching a new kind of exception. The latest push fixes it, and here's the green Mnh job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6958c596b3f4ad61750d4b139fb48b68fec871a8&selectedJob=74007211

Once the two dependent bugs are fixed, I'll format this for review and submit it.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Yay, I'm glad this is happening. Looking at your try run, I see that the pytest-mozlog plugin isn't being picked up [2]. Compare with logs in this job: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=marionette%20harness&selectedJob=74190112

The pytest-mozlog plugin just changes pytest's output so that it matches log parser expectations for Treeherder job status, log viewer, failure summary and such. 

Because of how the pytest-mozlog plugin is packaged in mozlog [1], you need to run mozlog's setup.py for pytest to pick the plugin up. (It was done this way so that whenever mozlog is installed, pytest should notice pytest-mozlog so you don't have to install pytest-mozlog separately.) I played around with this a while back and found that I can get it working in mach by changes in the appropriate package.txt file(s) as follows:

-mozlog.pth:testing/mozbase/mozlog
+setup.py:testing/mozbase/mozlog:develop

On a related note, it sounds like you're doing something that would also fix Bug 1302204, or make it obsolete?


[1] https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/testing/mozbase/mozlog/setup.py#36-37
[2] See also https://bugzilla.mozilla.org/show_bug.cgi?id=1302204#c2
Flags: needinfo?(ahalberstadt)
Sure I can modify this to use the pytest-mozlog format instead. Personally I like the default pytest format better, but you are correct that it doesn't produce results in the errorsummary.. and I'm not sure it's worth the effort to fix that. Would it be possible to add "--log-tbpl -" to the pytest.main invocation (like the mozharness script does) instead of changing packages.txt?

Yes, after this patch neither the marionette-harness kind nor mozharness script exist, so bug 1302204 would be obsolete.
Flags: needinfo?(ahalberstadt)
These jobs should ultimately be Tier-1, so I assume that means the error summary is important and helpful to sheriffs, but there might be a better way to get there. 

> Would it be possible to add "--log-tbpl -" to the pytest.main invocation (like the mozharness script does) instead of changing packages.txt?

Yes, we need to add --log-tbpl to pytest.main, but that relies on the plugin being available to pytest in the first place. I think the only difference between mozharness and mach here is how dependencies are set up: the mozharness virtualenv setup step calls mozlog's setup.py.
Comment on attachment 8838117 [details]
Bug 1335873 - Convert marionette harness unittests to standard python unittests,

https://reviewboard.mozilla.org/r/113104/#review114950

r+wc

::: taskcluster/ci/source-check/python-tests.yml:52
(Diff revision 2)
> +        - release
> +    when:
> +        files-changed:
> +          - 'testing/marionette/harness/**'
> +          - 'testing/mozbase/mozlog/mozlog/pytest_mozlog/**'
> +          - 'testing/config/marionette_harness_test_requirements.txt'

This file was only used by the now-deleted marionette-harness mozharness script, so it can also be deleted, too.
Attachment #8838117 - Flags: review?(mjzffr) → review+
Comment on attachment 8832893 [details]
Bug 1335873 - Ignore MozconfigFindException when looking for a mozinfo.json,

https://reviewboard.mozilla.org/r/109142/#review114972

Changing my review to r- since running these tests without an obj-dir fails locally: `./mach clobber && ./mach python-test --subsuite marionette-harness` results in `unrecognized arguments: --log-tbpl=-`. 

(`./mach build && ./mach python-test --subsuite marionette-harness` works fine.)
Attachment #8832893 - Flags: review?(mjzffr) → review-
Comment on attachment 8838117 [details]
Bug 1335873 - Convert marionette harness unittests to standard python unittests,

https://reviewboard.mozilla.org/r/113104/#review114980
Attachment #8838117 - Flags: review+ → review-
Comment on attachment 8838118 [details]
Bug 1335873 - Add marionette-harness python tests to root moz.build file,

https://reviewboard.mozilla.org/r/113106/#review114992

LGTM!
Attachment #8838118 - Flags: review?(mshal) → review+
Comment on attachment 8832893 [details]
Bug 1335873 - Ignore MozconfigFindException when looking for a mozinfo.json,

https://reviewboard.mozilla.org/r/109142/#review115414

::: testing/mozbase/mozinfo/mozinfo/mozinfo.py:229
(Diff revision 4)
>          if _os.path.isfile(json_path):
>              update(json_path)
>              return json_path
>      except ImportError:
>          pass
> -    except BuildEnvironmentNotFoundException:
> +    except (BuildEnvironmentNotFoundException, MozconfigFindException):

To add to our previous conversation on IRC, we only hit MozconfigFindException if there's no objdir __and__ there's a MOZCONFIG env variable set.
Attachment #8832893 - Flags: review?(mjzffr) → review+
Comment on attachment 8838117 [details]
Bug 1335873 - Convert marionette harness unittests to standard python unittests,

https://reviewboard.mozilla.org/r/113104/#review115412

r+wc

::: testing/mozbase/packages.txt:11
(Diff revision 3)
>  mozfile.pth:testing/mozbase/mozfile
>  mozhttpd.pth:testing/mozbase/mozhttpd
>  mozinfo.pth:testing/mozbase/mozinfo
>  mozinstall.pth:testing/mozbase/mozinstall
>  mozleak.pth:testing/mozbase/mozleak
> -mozlog.pth:testing/mozbase/mozlog
> +setup.py:testing/mozbase/mozlog:develop

Could you add a comment (or a note in the commit message) explaining why/that this is necessary for pytest-mozlog?
Attachment #8838117 - Flags: review?(mjzffr) → review+
Comment on attachment 8838117 [details]
Bug 1335873 - Convert marionette harness unittests to standard python unittests,

https://reviewboard.mozilla.org/r/113104/#review115412

> Could you add a comment (or a note in the commit message) explaining why/that this is necessary for pytest-mozlog?

I don't think the packages.txt files support comments, but I'll amend a note to the commit message.
So while changing mozlog from 'pth' to 'setup.py' worked locally, it seems to trigger a bug in the mach bootstrap in automation (see first push):
https://treeherder.mozilla.org/#/jobs?repo=try&author=ahalberstadt@mozilla.com&fromchange=cd237108541c0216734fde643b4aba8b0529e2b4&tochange=e702e139c0353b83ec717d21c0d5d3e604c85d02

I was able to fix the bug with the commit in the second push. I don't really understand why the change to 'setup.py' would trigger this, but the fix seems pretty reasonable. I'll get it up for review shortly.
Comment on attachment 8839522 [details]
Bug 1335873 - Check for .pyc's existence before deleting it in mach_bootstrap import hook,

https://reviewboard.mozilla.org/r/114138/#review117638
Attachment #8839522 - Flags: review?(mh+mozilla) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/caacf82afba1
Ignore MozconfigFindException when looking for a mozinfo.json, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/3c0a7527608a
Convert marionette harness unittests to standard python unittests, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/f35554b2951e
Add marionette-harness python tests to root moz.build file, r=mshal
https://hg.mozilla.org/integration/autoland/rev/2873be3659a3
Check for .pyc's existence before deleting it in mach_bootstrap import hook, r=glandium
Backed out for failing spidermonkey pkg due to not finding new file python.ini:

https://hg.mozilla.org/integration/autoland/rev/d725ecdbe54e6f3e39f86cc99f8e638d6b614431
https://hg.mozilla.org/integration/autoland/rev/29f59f3f6882445a6782a59ab55fe9473555f2ce
https://hg.mozilla.org/integration/autoland/rev/d2b0ed8f69e0c58328b772d235d4202103af3497
https://hg.mozilla.org/integration/autoland/rev/90ac7081a45fbccba3cb35e0b49f098b50d0f11c

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2873be3659a3841a44d652f2e84f25983c59c4fa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80864394&repo=autoland

[task 2017-03-01T14:53:40.341113Z] ==============================
[task 2017-03-01T14:53:40.341150Z] ERROR PROCESSING MOZBUILD FILE
[task 2017-03-01T14:53:40.341183Z] ==============================
[task 2017-03-01T14:53:40.341205Z] 
[task 2017-03-01T14:53:40.341240Z] The error occurred while processing the following file:
[task 2017-03-01T14:53:40.341260Z] 
[task 2017-03-01T14:53:40.341295Z]     /home/worker/workspace/sm-package/mozjs-54.0a1.0/moz.build
[task 2017-03-01T14:53:40.341317Z] 
[task 2017-03-01T14:53:40.341350Z] The error was triggered on line 92 of this file:
[task 2017-03-01T14:53:40.341371Z] 
[task 2017-03-01T14:53:40.341416Z]     'testing/marionette/harness/marionette_harness/tests/harness_unit/python.ini',
[task 2017-03-01T14:53:40.341437Z] 
[task 2017-03-01T14:53:40.341486Z] An error was encountered as part of executing the file itself. The error appears to be the fault of the script.
[task 2017-03-01T14:53:40.341506Z] 
[task 2017-03-01T14:53:40.341534Z] The error as reported by Python is:
[task 2017-03-01T14:53:40.341553Z] 
[task 2017-03-01T14:53:40.341618Z]     ['IOError: Missing files: /home/worker/workspace/sm-package/mozjs-54.0a1.0/testing/marionette/harness/marionette_harness/tests/harness_unit/python.ini\n']
[task 2017-03-01T14:53:40.341639Z] 
[task 2017-03-01T14:53:40.360343Z] Traceback (most recent call last):
[task 2017-03-01T14:53:40.360395Z]   File "./devtools/automation/autospider.py", line 295, in <module>
[task 2017-03-01T14:53:40.360423Z]     run_command(['sh', '-c', posixpath.join(PDIR.js_src, 'configure') + ' ' + CONFIGURE_ARGS], check=True)
[task 2017-03-01T14:53:40.360446Z]   File "./devtools/automation/autospider.py", line 270, in run_command
[task 2017-03-01T14:53:40.360471Z]     raise subprocess.CalledProcessError(status, command, output=stderr)
[task 2017-03-01T14:53:40.360529Z] subprocess.CalledProcessError: Command '['sh', '-c', u'/home/worker/workspace/sm-package/mozjs-54.0a1.0/js/src/configure  --enable-optimize --enable-nspr-build --prefix=/home/worker/workspace/sm-package/mozjs-54.0a1.0/obj-spider/dist']' returned non-zero exit status 1
Flags: needinfo?(ahalberstadt)
Looks like I needed to put the manifest behind an `if not CONFIG['JS_STANDALONE']` variable. The latest patch series fixes it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb6cb3bfdd4caa10d373497f2704c917260d7a5e
Flags: needinfo?(ahalberstadt)
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7f470890384
Ignore MozconfigFindException when looking for a mozinfo.json, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/42c74dd948b9
Convert marionette harness unittests to standard python unittests, r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/290234233d6b
Add marionette-harness python tests to root moz.build file, r=mshal
https://hg.mozilla.org/integration/autoland/rev/d23c02b2fed0
Check for .pyc's existence before deleting it in mach_bootstrap import hook, r=glandium
Blocks: 1345109
Blocks: 1347483
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: