marionette automated test support for Thunderbird
Categories
(Thunderbird :: Testing Infrastructure, task)
Tracking
(Not tracked)
People
(Reporter: samuel.thibault, Assigned: samuel.thibault)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 2 obsolete files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
6.39 KB,
patch
|
rjl
:
review+
|
Details | Diff | Splinter Review |
3.23 KB,
patch
|
Details | Diff | Splinter Review | |
4.08 KB,
patch
|
darktrojan
:
review+
samuel.thibault
:
feedback+
|
Details | Diff | Splinter Review |
Hello,
As discussed on http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2019-March/001469.html we would like to use marionette to make UI regression testing in thunderbird. I will submit phabricator changesets for this.
Samuel
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Hmm, you have two patches here, one for M-C and the other one for C-C. The M-C one hard-codes TB stuff into M-C, like
# Do not start first-run items
"mail.provider.suppress_dialog_on_startup": True,
"mail.spotlight.firstRunDone": True,
"mail.winsearch.firstRunDone": True,
# Do not open start page
"mailnews.start_page.override_url": "about:blank",
"mailnews.start_page.url": "about:blank",
That looks a little unfortunate. In case we have to change C-C stuff, we need to need an M-C patch to do to. I'd prefer Rob and or Geoff to look at this before landing it.
Comment 4•6 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #3)
That looks a little unfortunate. In case we have to change C-C stuff, we need to need an M-C patch to do to. I'd prefer Rob and or Geoff to look at this before landing it.
Yeah, that's not ideal. It would be nice to move ThunderbirdInstance to someplace in C-C.
IF Marionette tests work like Mochitests, we might be able to store that thunderbird_prefs dictionary in a JSON file under C-C. In CI, that relies on an artifact file being updated and transferred into the test environment; not sure if that happens in this case.
(In reply to SamuelThibault on Phabricator)
(the try didn't actually try marionette, I guess the comm try server will need to be taught to do it)
Yes, similar to what had to be done with Mochitest (bug 1507317), new test tasks need to be created under comm/taskcluster/ci/.
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
I have changed the implementation by moving thunderbird_prefs to a module in testing/marionette/thunderbird/thunderbirdinstance.py .
I have added the taskcluster/ci infrastructure, that indeed triggers the tests on the try server, as seen on
There is however one issue: it seems the 2828 port doesn't get opened: the live.log doesn't say "Listening on port 2828", and eventually it says "Process killed after 120s because no connection to Marionette server could be established".
I guess this is because of one issue I met: when mail.provider.suppress_dialog_on_startup is left to False, on startup thunderbird begins with asking for a mail account, and it happens that the marionette server doesn't start before this, one has to close that dialog to get port 2828 opened. That is why I set it to True in thunderbird_prefs, and that does work well when using ./mach marionette-test. I am however wondering whether perhaps the marionette.py script called from the CI script does not do this preference initialization, and thus doesn't get this fixed.
Any idea on that issue?
I guess that at least the M-C part can go in.
Assignee | ||
Comment 6•6 years ago
|
||
I forgot to mention for the M-C part that it was successfully tested on
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29506af3623710246341979897cb7e884796d33a
Assignee | ||
Comment 7•6 years ago
|
||
Ok, I guess the preference initialization doesn't happen just because thunderbirdinstance.py is not getting shipped in the tarballs.
Actually, I wonder how comm/ files get into the tarballs. I only see xpcshell and mochitest files getting installed. I was assuming that adding the pieces in moz.build and manifest.ini would get the files installed in the test environment, but that doesn't seem to be the case.
What makes the comm/ files installed for the xpcshell/mochitest cases actually? I can have a look at replicating it, but for now I'm a bit clueless as to how this happens in the xpcshell/mochitest case.
Comment 8•6 years ago
|
||
(In reply to Samuel Thibault from comment #7)
What makes the comm/ files installed for the xpcshell/mochitest cases actually? I can have a look at replicating it, but for now I'm a bit clueless as to how this happens in the xpcshell/mochitest case.
I think you're looking for this?
https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/action/test_archive.py#625-633
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Rob Lemley [:rjl] from comment #8)
I think you're looking for this?
https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/action/test_archive.py#625-633
Thanks! Exactly what I was looking for :)
Assignee | ||
Comment 10•6 years ago
|
||
The latest version on phab went fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=471ff1dd8fbce5fa11e9de325cbe772c84d3c74c
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=91fb97286685562ca455ab57e05638256f6657b0
Assignee | ||
Comment 11•6 years ago
|
||
So, how does it feel now with the rework?
I need it for the testcase of https://bugzilla.mozilla.org/show_bug.cgi?id=1546650
Comment 12•6 years ago
|
||
It looks good since the TB prefs are in C-C now. I'm a little confused with respect to the relevant changes, I think:
M-C: https://hg.mozilla.org/try/rev/88f9540db7c8
C-C: https://hg.mozilla.org/try-comm-central/rev/dc8c74a8bfa2 or is it https://hg.mozilla.org/try-comm-central/rev/2bfa09effa26.
(and if you paste links as text, they get linkified).
Assignee | ||
Comment 13•6 years ago
|
||
It's https://hg.mozilla.org/try-comm-central/rev/2bfa09effa26 , the other just additionally include the .gecko_rev.yml change to pull the M-C change for the try server to be able to really test it.
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
I don't see a question here. Rob (:rjl) can review the C-C changes and you need to get another review for the M-C part.
Assignee | ||
Comment 15•6 years ago
|
||
The M-C part can be reviewed by ato I guess?
Comment 16•6 years ago
|
||
I'd remove the bits for the platforms that we aren't building for at the moment, otherwise looks good to me.
Updated•6 years ago
|
Comment 17•6 years ago
|
||
(In reply to Samuel Thibault from comment #15)
The M-C part can be reviewed by ato I guess?
Yes, certainly.
Although do note that I’m not actively working on Marionette this
year, so my response time to reviews will be influenced by this
fact.
Assignee | ||
Comment 18•6 years ago
|
||
Well, I have to say I don't know who I am supposed to ping for this, I just happen to look at the hg history, I haven't found anything more precise from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction#Step_4_Get_your_code_reviewed in the marionette case.
Comment 19•6 years ago
|
||
What's the problem? Andreas (:ato) offered to re-review the new M-C patch, which is just a (stripped-down?) variation on what was already approved. Rob (:rjl) will review the C-C part, and he already made a suggestion in comment #16 to finalise the patch. Can we proceed like that?
Assignee | ||
Comment 20•6 years ago
|
||
Sure, I'm just wondering who I was supposed to ping if ato is not active on Marionette. Or is perhaps ato actually the right person even if he is not currently active on it? Finding who I am supposed to set as a reviewer looks relatively obscure to me, I am used to projects (qemu, linux, etc.) which have identified maintainers for the different files.
comment #16 was addressed.
Comment 21•6 years ago
|
||
(In reply to Samuel Thibault from comment #20)
Sure, I'm just wondering who I was supposed to ping if ato is not
active on Marionette. Or is perhaps ato actually the right person
even if he is not currently active on it?
At present, there is no active feature development work going on
for testing/marionette, but it is still a maintained component.
Maintenance work is influenced by factors such as OKRs and department
goals, but I didn’t mean to say I wouldn’t be responsive to reviews
(at least I think I didn’t).
The reviewer list for testing/marionette is:
- Andreas Tolfsen [:ato]
- Henrik Skupin [:whimboo]
- David Burns [:AutomatedTester]
- Maja Frydrychowicz [:maja_zf]
Finding who I am supposed to set as a reviewer looks relatively
obscure to me, I am used to projects (qemu, linux, etc.) which have
identified maintainers for the different files.
Generally speaking, the module ownership system at Mozilla is
broken. What you suggested earlier, to look at the history/blame
log of a particular file is absolutely the right thing to do.
Assignee | ||
Comment 22•6 years ago
|
||
Ok, thanks for the clarification :)
Updated•6 years ago
|
Assignee | ||
Comment 23•5 years ago
|
||
Rob, could you check that https://phabricator.services.mozilla.com/D27104 now addresses your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1543725#c16 ?
Assignee | ||
Comment 25•5 years ago
|
||
Ok, thanks!
So Andreas, could you check the reworked https://phabricator.services.mozilla.com/D27102 ?
Comment 26•5 years ago
|
||
Yes, this still looks fine to me.
If I understand this correctly, you will put the tests under
comm/testing/marionette/thunderbird/, not under
testing/marionette/thunderbird/. This was my principal concern.
Why do we need comm/testing/marionette/thunderbird/
and not just comm/testing/marionette/
? Is it because of SeaMonkey that this separation is needed?
Assignee | ||
Comment 28•5 years ago
|
||
I must admit that I don't remember. I guess I thought about something like comm/ not necessarily only containing the thunderbird application, indeed.
Comment 29•5 years ago
|
||
Could be preferable to still just use comm/testing/marionette/
Assignee | ||
Comment 30•5 years ago
|
||
Ok, I have checked that it works fine without the thunderbird/ subdir, and updated the phab.
Is it now ready for checkin?
Assignee | ||
Comment 32•5 years ago
|
||
Ok, so let's do it :)
To the gentle commiter:
- D27102 is for the M-C repo
- D27104 is for the C-C repo and depends on D27102.
Thanks!
Comment 33•5 years ago
|
||
Hmm, the M-C patch was updated after the last review and just had some more comments. Can you please address those and get a re-review.
Sorry for the late reply on that. I actually don't want to disturb the landing process, but some things aren't that clear to me yet. And I haven't seen those patches earlier. :/
Beside that it is great to see that we finally make progress with Marionette, and that there will be new tests hopefully soon. Getting rid of Mozmill also for Thunderbird would be a really great achievement.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 35•5 years ago
|
||
To the gentle commiter:
D27102 is for the M-C repo
D27104 is for the C-C repo and depends on D27102.
Thanks!
Updated•5 years ago
|
Comment 36•5 years ago
|
||
I've triggered the landing of the M-C part now.
Comment 37•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/integration/autoland/rev/bb3d70c37398
Add marionette support to thunderbird r=ato,whimboo
Updated•5 years ago
|
Comment 38•5 years ago
|
||
bugherder |
Comment 39•5 years ago
|
||
Comment 40•5 years ago
|
||
https://hg.mozilla.org/comm-central/rev/49d7adb910b066131e595798ec83a73fee457d07
Backed out changeset 4058126a02ed (bug 1543725) for build bustage. a=backout
Comment 41•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c76024e437fd
Add marionette support to Thunderbird, empty for now. r=rjl,whimboo
Comment 42•5 years ago
|
||
Sorry for the noise here, but this patch does break the build. It's broken again after landing it again, however, it worked on try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c76024e437fd54f0870071df90041f6e6aab74ab
Comment 43•5 years ago
|
||
Comment 44•5 years ago
|
||
Backout from M-C:
https://hg.mozilla.org/mozilla-central/rev/d5226e5dc522af70e16172029e8c8286e2a8674c
Backout from C-C:
https://hg.mozilla.org/comm-central/rev/0a9583f964a9bd49b10279c486f55066d18c9642
Updated•5 years ago
|
Assignee | ||
Comment 45•5 years ago
|
||
Mmm, just thinking: perhaps we need to first land the testing/marionette/unit-tests.ini and testing/marionette/thunderbirdinstance.py files in c-c, and then add the rules shipping them in python/mozbuild/mozbuild/action/test_archive.py in m-c, and then only we can enable their use in c-c?
Comment 46•5 years ago
|
||
The bustage is happening in the Decision task during the task optimization phase, so well before anything from test_archive.py becomes relevant. It looks like it's related to the sparsecheckout that gets done in a decision task. I've been able to reproduce it locally now and I'm stepping through a debugger trying to figure out exactly what's going on.
Assignee | ||
Comment 47•5 years ago
|
||
Rob, do you think you will be able to look at it?
Assignee | ||
Updated•5 years ago
|
Comment 48•5 years ago
|
||
Well, I sort of made some progress.
Somehow we're triggering the decision task to go through and process all of the moz.build files. I think it's got something to do with SCHEDULES.inclusive in the moz.build file at the root of M-C. Anyway, as it's doing that walk, it tries to read comm/moz.build which exists on the filesystem and is readable and all of that good stuff. However, it's trying to read that file out of M-C's Mercurial repository database, and of course it's not there.
The code handling this is https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/python/mozbuild/mozpack/hg.py#79-86
def get(self, path):
if self._recognize_repo_paths:
if not path.startswith(self._root):
raise ValueError('lookups in recognize_repo_paths mode must be '
'prefixed with repo path: %s' % path)
path = path[len(self._root) + 1:]
return self._get(path)
self._root is going to be $topsrcdir (M-C's root) and path in this case is "$topsrcdir/comm/moz.build". So the path.startswith() test passes. Then self._get(path) call uses hglib to attempt to read the file from the repository at $topsrcdir/.hg. That fails, and we get our "The underlying problem is a referenced path could not be read." error.
More to come..
Comment 49•5 years ago
|
||
This is triggering Bug 1414043.
That's going to be a nightmare. Another option is to not use "SCHEDULES" by using the "when" or "optimization" parameters. Looking at that.
Comment 50•5 years ago
|
||
Updated•5 years ago
|
Comment 51•5 years ago
|
||
Comment 52•5 years ago
|
||
Comment 53•5 years ago
|
||
I had mentioned in IRC that I was going to look at the M-C patch again with regard to not breaking Thunderbird builds with the patch present in M-C but not the corresponding C-C pieces.
After looking at it, I'm of the opinion that a build fail is what we want. The error is that the marionette test manifest file can't be read. Under normal circumstances, I would say that if a test manifest is not present or presents some other error, then there should be a build failure so it can be addressed.
With my additional patch (which goes before my modified version of Sam's C-C patch) this should be ready at long last. I checked the M-C patch, and it still applys cleanly, so hopefully that can land without problems.
Updated•5 years ago
|
Comment 54•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 55•5 years ago
|
||
Here is rjl's patch with darktrojan's comment applied.
Assignee | ||
Comment 56•5 years ago
|
||
Ok, we should now have all the pieces ready for commit. This should go this way:
- first move_test_job_defaults2.patch to C-C
- then attachment 9057573 [details] (i.e. phab's D27102) to M-C
- then minor_updates_to_marionetta_cc.patch to C-C
It needs to go this way because the last patch depends on the second, and the second breaks the build unless the first is applied first.
Thanks everybody!
Updated•5 years ago
|
Comment 57•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3d07be225714
Use skip-unless-changed scheduling for all included test yml files. r=darktrojan
Updated•5 years ago
|
Comment 58•5 years ago
|
||
(In reply to Samuel Thibault from comment #56)
- first move_test_job_defaults2.patch to C-C
- then attachment 9057573 [details] (i.e. phab's D27102) to M-C
- then minor_updates_to_marionetta_cc.patch to C-C
I landed the first part and will land the M-C piece now. What about D27104? That landed on M-C before.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 59•5 years ago
|
||
Sorry, got it, minor_updates_to_marionetta_cc.patch is a modified version of D27104.
Comment 60•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/integration/autoland/rev/12fef65e97c9
Add marionette support to thunderbird r=ato,whimboo
Comment 61•5 years ago
|
||
Comment 62•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 63•5 years ago
|
||
bugherder |
Comment 64•5 years ago
|
||
Comment 65•5 years ago
|
||
You got a bunch of Marionette failures now:
Process killed after 120s because no connection to Marionette server could be established. Check gecko.log for errors
Assignee | ||
Comment 66•5 years ago
|
||
Bleh? Never had those when running on the try server... where do you see them?
I guess that until we get to fix these, backing off D27104 would avoid the issue?
Comment 67•5 years ago
|
||
I see them on the treeherder on Mac jobs:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=f5f85315ba00b83e9a290aeeeff80eac5176c881
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=f5f85315ba00b83e9a290aeeeff80eac5176c881&selectedJob=254166748
I'm not backing anything out to confuse this bug even more. Since failing or passing the tests means nothing at the moment, we can just ignore the oranges.
Note that for those failing jobs I cannot see that the marionette-startup-requested
observer notification is received. As such Marionette never starts-up the socket server, and the client times while waiting for the connection.
Assignee | ||
Comment 69•5 years ago
|
||
Indeed.
The marionette support itself is compiled in (ENABLE_MARIONETTE is set to 1)
Interestingly, when looking at the mochitest log, marionette properly gets initialized etc. so the support definitely is there and available for running.
I remember seeing this kind of issue in the Marionette run when thunderbird was busy with showing startup dialog boxes such as first-time configuration, thus the options in comm/testing/marionette/thunderbirdinstance.py , perhaps some more is needed on macos due to some OS specificity?
Perhaps somebody with a macos system can run the build to check what kind of startup dialogs or something else show up and might be keeping thunderbird busy?
Updated•5 years ago
|
Comment 70•5 years ago
|
||
Comment 71•5 years ago
|
||
So if you back this out as was suggested, you kill the build:
[task 2019-07-06T10:31:53.806Z] 10:31:53 INFO - package-tests> File "/builds/worker/workspace/build/src/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 269, in read
[task 2019-07-06T10:31:53.807Z] 10:31:53 INFO - package-tests> raise IOError('Missing files: %s' % ', '.join(missing))
[task 2019-07-06T10:31:53.807Z] 10:31:53 INFO - package-tests> IOError: Missing files: /builds/worker/workspace/build/src/comm/testing/marionette/unit-tests.ini
[task 2019-07-06T10:31:53.807Z] 10:31:53 INFO - package-tests> /builds/worker/workspace/build/src/testing/testsuite-targets.mk:164: recipe for target 'package-tests-common' failed
[task 2019-07-06T10:31:53.807Z] 10:31:53 ERROR - package-tests> make[2]: *** [package-tests-common] Error 1
How many hours have I spent now managing this :-(
Comment 72•5 years ago
|
||
Assignee | ||
Comment 73•5 years ago
|
||
Only the last change which really introduce a test should have been backed out. The first c-c change was needed before the m-c change.
Comment 74•5 years ago
|
||
Yes. I backed out the "last" change in rev f5f85315ba00 together with the white-space follow-up in rev 7a364236efd0.
That's when the build started to fail, see comment #71 and
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=22be14b4e4fac49c2af45d306ac19b96b8e1e60b
The very first C-C part wasn't backed out and neither the M-C part, but it looks like the M-C part unconditionally requires certain files from the last C-C part:
https://hg.mozilla.org/mozilla-central/rev/12fef65e97c9#l3.83
So a complete backout of that last part breaks the build.
Comment 75•5 years ago
|
||
These are the hunks which weren't relanded. Relanding this will get us back to the status where marionette works on Linux and Windows, but fails on Mac.
Comment 76•5 years ago
|
||
Maybe I can disable the mac tests at least until the stray dialog or whatever gets figured out?
Assignee | ||
Comment 77•5 years ago
|
||
Yes, we could do disable it on mac.
Since I don't have a macos system I just tried various configuration bits I could find, but without success, so I'm out of ideas to make it work on macos.
Assignee | ||
Comment 78•5 years ago
|
||
(I don't really know how to disable just marionette.yml on mac, I tried a few things without success)
Updated•5 years ago
|
Comment 79•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 80•5 years ago
|
||
Assignee | ||
Comment 81•5 years ago
|
||
Assignee | ||
Comment 82•5 years ago
|
||
So I believe we can push https://bugzilla.mozilla.org/attachment.cgi?id=9080775 (enable_marionette_without_macos.patch)
Comment 83•5 years ago
|
||
Thanks for taking care of it, Rob. Can someone file a follow-up bug to get it working on Mac?
Updated•5 years ago
|
Comment 84•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d1b9dbba6dbf
Enable marionette tests on Windows/Linux. r=darktrojan CLOSED TREE DONTBUILD
Assignee | ||
Comment 85•5 years ago
|
||
I will!
Assignee | ||
Comment 86•5 years ago
|
||
Done so as Bug 1569118
Description
•