Closed Bug 1543725 Opened 4 months ago Closed 29 days ago

marionette automated test support for Thunderbird

Categories

(Thunderbird :: Testing Infrastructure, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 69.0

People

(Reporter: samuel.thibault, Assigned: samuel.thibault)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

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

Type: defect → task
Keywords: checkin-needed

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.

Flags: needinfo?(rob)
Flags: needinfo?(geoff)
Keywords: checkin-needed

(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/.

Flags: needinfo?(rob)
Assignee: nobody → samuel.thibault
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cc4dac9089479c53b7ba372799bd732467ea4bb0&selectedJob=240358399

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.

I forgot to mention for the M-C part that it was successfully tested on

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29506af3623710246341979897cb7e884796d33a

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.

(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

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

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

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

Flags: needinfo?(rob)
Flags: needinfo?(jorgk)

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).

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)

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.

Flags: needinfo?(jorgk)

The M-C part can be reviewed by ato I guess?

Flags: needinfo?(ato)

I'd remove the bits for the platforms that we aren't building for at the moment, otherwise looks good to me.

Flags: needinfo?(rob)
Summary: Adding marionette support → marionette automated test support for Thunderbird

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

Flags: needinfo?(ato)

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.

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?

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.

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

Ok, thanks for the clarification :)

Attachment #9057575 - Attachment description: Bug 1543725 Add marionette support to thunderbird, empty for now r=ato → Bug 1543725 Add marionette support to thunderbird, empty for now r=rjl
Blocks: 802990
Flags: needinfo?(rob)

Looks good, thanks.

Flags: needinfo?(rob)

Ok, thanks!

So Andreas, could you check the reworked https://phabricator.services.mozilla.com/D27102 ?

Flags: needinfo?(ato)

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.

Flags: needinfo?(ato)

Why do we need comm/testing/marionette/thunderbird/ and not just comm/testing/marionette/? Is it because of SeaMonkey that this separation is needed?

I must admit that I don't remember. I guess I thought about something like comm/ not necessarily only containing the thunderbird application, indeed.

Could be preferable to still just use comm/testing/marionette/

Ok, I have checked that it works fine without the thunderbird/ subdir, and updated the phab.
Is it now ready for checkin?

Flags: needinfo?(mkmelin+mozilla)

I think so

Flags: needinfo?(mkmelin+mozilla)

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!

Keywords: checkin-needed

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.

Keywords: checkin-needed
Attachment #9057575 - Attachment description: Bug 1543725 Add marionette support to thunderbird, empty for now r=rjl → Bug 1543725 Add marionette support to thunderbird, empty for now r=rjl,whimboo
Attachment #9057573 - Attachment description: Bug 1543725 Add marionette support to thunderbird r=ato → Bug 1543725 Add marionette support to thunderbird r=ato,whimboo

To the gentle commiter:

D27102 is for the M-C repo
D27104 is for the C-C repo and depends on D27102.

Thanks!

Keywords: checkin-needed
Flags: needinfo?(geoff)

I've triggered the landing of the M-C part now.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/integration/autoland/rev/bb3d70c37398
Add marionette support to thunderbird r=ato,whimboo

Keywords: checkin-needed
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4058126a02ed
Add marionette support to Thunderbird, empty for now. r=rjl,whimboo

https://hg.mozilla.org/comm-central/rev/49d7adb910b066131e595798ec83a73fee457d07
Backed out changeset 4058126a02ed (bug 1543725) for build bustage. a=backout

Status: RESOLVED → REOPENED
Flags: needinfo?(samuel.thibault)
Resolution: FIXED → ---

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c76024e437fd
Add marionette support to Thunderbird, empty for now. r=rjl,whimboo

Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED

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

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/63e4680def0f
remove 'test/marionette' from mail/moz.build in an attempt to fix the build. rs=bustage-fix
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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?

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.

Rob, do you think you will be able to look at it?

Flags: needinfo?(samuel.thibault) → needinfo?(rob)

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..

Flags: needinfo?(rob)

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.

Attached patch move_test_job_defaults.patch (obsolete) — Splinter Review
This will need to land on C-C before Sam's patch.
Attachment #9074221 - Flags: review?(geoff)
Assignee: samuel.thibault → rob
Status: REOPENED → ASSIGNED
Update to Sam's C-C patch, D27104.

- Rebase onto attachment 9074221 [details] [diff] [review]
- Add license header to marionette.yml
Comment on attachment 9074223 [details] [diff] [review]
minor_updates_to_marionetta_cc.patch

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

This is just a rebase and license header addition to Sam's work.
Attachment #9074223 - Flags: review+

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.

Assignee: rob → samuel.thibault
Comment on attachment 9074221 [details] [diff] [review]
move_test_job_defaults.patch

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

::: taskcluster/ci/test/tests.yml
@@ +12,5 @@
>  # Adding a new test type or running tests on a new platform? Be sure to review
>  # https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy
> +
> +# scheduling via "when" in job-defaults moved to kind.yml to facilitate
> +# splitting into multiple test yml files.

We don't really need a comment about what used to be here. We have |hg log| for that.
Attachment #9074221 - Flags: review?(geoff) → review+
Attachment #9057575 - Attachment is obsolete: true

Here is rjl's patch with darktrojan's comment applied.

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!

Keywords: checkin-needed
Keywords: leave-open

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

Keywords: checkin-needed
Attachment #9074221 - Attachment is obsolete: true

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

Flags: needinfo?(samuel.thibault)
Flags: needinfo?(rob)
Attachment #9074964 - Attachment description: move_test_job_defaults2.patch → move_test_job_defaults2.patch [landed in comment #57]
Keywords: checkin-needed

Sorry, got it, minor_updates_to_marionetta_cc.patch is a modified version of D27104.

Flags: needinfo?(samuel.thibault)
Flags: needinfo?(rob)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/integration/autoland/rev/12fef65e97c9
Add marionette support to thunderbird r=ato,whimboo

Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5d1b0dbe9f0c
Follow-up: Fix yaml issue. rs=white-space-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f5f85315ba00
Add marionette support to Thunderbird, empty for now. r=rjl,whimboo
Attachment #9057575 - Attachment is obsolete: false
Status: ASSIGNED → RESOLVED
Closed: 3 months ago2 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 69.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7a364236efd0
Follow-up: Fix yaml issue. rs=white-space-only DONTBUILD

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

Status: RESOLVED → REOPENED
Flags: needinfo?(samuel.thibault)
Flags: needinfo?(rob)
Resolution: FIXED → ---

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?

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.

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?

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/22be14b4e4fa
Backed out 2 changesets for perma-failures on Mac. a=backout DONTBUILD

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

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ea8da6cabe7a
Reland all new files to get the build going again. a=me DONTBUILD

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.

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.

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.

Maybe I can disable the mac tests at least until the stray dialog or whatever gets figured out?

Flags: needinfo?(rob)

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.

(I don't really know how to disable just marionette.yml on mac, I tried a few things without success)

Attachment #9076363 - Attachment is obsolete: true
Assignee: samuel.thibault → rob
Status: REOPENED → ASSIGNED
Assignee: rob → samuel.thibault
Comment on attachment 9080775 [details] [diff] [review]
enable_marionette_without_macos.patch

Looks like it should work.
Attachment #9080775 - Flags: review?(geoff) → review+
Comment on attachment 9080775 [details] [diff] [review]
enable_marionette_without_macos.patch

This seems good indeed, thanks!
Flags: needinfo?(samuel.thibault)
Attachment #9080775 - Flags: feedback?(samuel.thibault) → feedback+

So I believe we can push https://bugzilla.mozilla.org/attachment.cgi?id=9080775 (enable_marionette_without_macos.patch)

Keywords: checkin-needed

Thanks for taking care of it, Rob. Can someone file a follow-up bug to get it working on Mac?

Keywords: leave-open

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d1b9dbba6dbf
Enable marionette tests on Windows/Linux. r=darktrojan CLOSED TREE DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 2 months ago29 days ago
Keywords: checkin-needed
Resolution: --- → FIXED

I will!

Done so as Bug 1569118

You need to log in before you can comment on or make changes to this bug.