1272145 - [16Q3] Move Firefox-ui-tests from testing/firefox-ui-tests/tests to individual browser/toolkit components

Status: RESOLVED WONTFIX

(Testing :: Firefox UI Tests, defect)

Description

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

Right now all of our Firefox ui tests (including the puppeteer unit tests) are located in testing/firefox-ui-tests/tests/. That means the tests are not that discoverable as they should for developers. To solve this issue we want to move the tests out of this folder into the appropriate browser/toolkit component folders.

The move action is easy, but collecting all the tests for the test archive doesn't seem to be. Right now we are using the test_archive.py script to package all of our tests. But once tests are spread around the tree this script does no longer work for us. It simply misses a collector for tests as referenced in a master manifest file.

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/test_archive.py

Chris or Gregory, what do you think is the best method here to collect tests and manifests, and package them correctly in the test archive? Maybe I miss some new features which recently got implemented?

Comment 1

[Gregory Szorc [:gps]]

Test archiving is kinda wonky. Basically test files are copied into objdir/_tests during the build and packaging. Tests magically end up there for the most part as a side-effect of having test manifests registered in moz.build files. The test archiver archives most test files from that location. But it can grab things from other locations too.

If you split things up, you'll need to add paths and patterns in test_archive.py if you change nothing else.

The preferred way to do this is to register the test manifests for these tests with moz.build files. That way, the test files will automatically get picked up by the build system and archived (or at least archived with a single pattern rule instead of N). For test harness files, you'll need to continue to define custom rules to archive them, as we don't yet have a mechanism in moz.build for declaring where files should get packaged in test archives. That's what the patterns in test_archive.py are for.

This is a bit confusing, I know. You can ping us in #build if you have more questions.

Comment 2

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

Thank's Gregory. I think that should give me a good start!

Comment 3

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

(In reply to Gregory Szorc [:gps] from comment #1)
> Test archiving is kinda wonky. Basically test files are copied into
> objdir/_tests during the build and packaging. Tests magically end up there
> for the most part as a side-effect of having test manifests registered in
> moz.build files. The test archiver archives most test files from that
> location. But it can grab things from other locations too.

If we would do it this way, what about the initiative from build faster in trying to copy as less as possible tests to that location? I don't want to add something which is actually counter productive for this other project. 

Chris, given that you work on build faster, what is your feedback?

Comment 4

[Chris Manchester (limited bugmail, email directly)]

I would encourage you not to sync the files to objdir/_tests for this reason, but if you have to, there are probably few enough that is would be a big problem.

Comment 5

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

Given that we do not have that many tests yet, I will go with copying to objdir for now. As a follow-up we could figure out how to improve test_archive.py to be able to collect manifest files and dirs across the tree.

Comment 6

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

So it looks like that when moving our tests to separate components we will loose the capability to run via the source dir, unless we duplicate the efforts of collecting manifest files for moz.build and our main manifest in testing/firefox-ui/tests like:

testing/firefox-ui/tests/functional.ini:
> [include:toolkit/components/autocompelete/.../manifest.ini]
> [include:toolkit/components/privatebrowsing/.../manifest.ini]

testing/firefox-ui/tests/update.ini:
> [include:toolkit/mozapps/update/tests/.../manifest.ini]

toolkit/mozapps/update/tests/moz.build:
> FIREFOX_UI_UPDATE_MANIFESTS += [
>     'firefox_ui/aus_update/aus_update.ini',
> ]

mozbuild/mozbuild/frontend/context.py:
>    'FIREFOX_UI_UPDATE_MANIFESTS': (ManifestparserManifestList, list,
>        """List of manifest files defining firefox-ui update tests.
>        """),

mozbuild/mozbuild/testing.py:
> FIREFOX_UI_UPDATE=('update', 'firefox-ui', '.', True)

The non moz.build related references could be used to still run our tests from the source tree, but that would require to get new tests always added to the global manifest file for the given test type.  I feel this is lots of duplicated code and maintenance work.

Chris, what do you think?

As alternative we could add a manifest parsing logic to test_archive.py which works like a crawler and collects all directories and tests in test_archive.py. The main manifest files would be under /testing/firefox-ui/ and include other manifests from various components. That wouldn't require any changes to the moz.build files. But I'm actually not sure what the future plan is in how to reference test manifests and files. Maybe you can give me that information?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Most discussion about the future of test archives has revolved around ultimately doing away with them and running all tests from a source checkout. This would make our issue of figuring out what to package go away, but I don't have an estimate for when this work will take place.

As an interim solution, we could adapt the strategy taken by other marionette tests (print-manifest-dirs.py) to test_archive.py, as discussed on irc last week.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Some of the layout- and loop tests already live elsewhere in the three, but are included in the testing/marionette/harness/marionette/tests/unit-tests.ini manifest (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/harness/marionette/tests/unit-tests.ini#L5).  The way these tests are organised is suboptimal, but it works.

Comment 9

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

(In reply to Andreas Tolfsen ‹:ato› from comment #8)
> marionette/tests/unit-tests.ini#L5).  The way these tests are organised is
> suboptimal, but it works.

Exactly, and that's something I do not like at all. Having to reference manifests of various test suites via the harness manifests is totally irritating. I will try the way via moz.build files also because as Chris mentioned to me on IRC yesterday those do not require a `mach build` step, but Marionette will take care of moving the tests to the obj/_tests dir.

Comment 10

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

Chris, when I use the moz.build method with an artifact build I can see that only the top manifest files are getting created under obj/_tests, and nothing else symlinked/copied. I have to manually run `mach build package-tests` to actually populate the target dir with the sub-manifests and tests. Is that something we have to fix for artifact builds? As it looks like this affects all test suites which are run via the obj dir.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Tests are synced by the mach commands for testing, for instance: https://dxr.mozilla.org/mozilla-central/rev/d6d4e8417d2fd71fdf47c319b7a217f6ace9d5a5/testing/mochitest/mach_commands.py#411

Comment 12

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

I had another conversation with Chris by last week during our all hands work week. We came to the conclusion that we want to keep the "run tests from source tree" mode, and not copy them to the obj dir. That means our firefox ui tests will follow the style of Marionette tests and copy their print manifests file to feed the archiver client with the list of manifests.

Comment 13

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

This bug had to be moved to Q3 this year.

Comment 14

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

I would like to do the decoupling of puppeteer tests first. So re-arranging the dependencies.

Comment 15

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

Wait, bug 1271804 is actually more than moving the tests only.

Comment 16

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

I started a discussion in dev.platform to get early feedback for the proposed locations of the tests. I will request ni? from module owners once larger questions and issues have been solved.

Comment 17

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

The discussion on dev.platform was not that successful. So I would like to continue here on the bug and first ask the module owners of Firefox and Toolkit for their feedback, and a list of possible peers who should also have a word about the final location of the specific tests.

In case you are not aware what our tests are doing, here a short overview: fx-ui tests are based on Marionette and are the successor of the QA maintained Mozmill tests. Basically those are integration or end2end tests for various components in browser and toolkit. They do mostly operate on UI elements, including the usage of DTD entities and properties to allow testing in localized builds even. We make less usage of Gecko API, and if we have to those are mostly necessary for setup and teardown logic of a test. Another benefit of our test are the restart capability of Firefox, which comes via Marionette.

Why we don't use Marionette itself? We have a page object model called Firefox Puppeteer which eases the creation and maintenance of tests by allowing to access elements and actions like self.browser.tabbar.open_tab(). Due to this addition a separate harness was necessary, and as result the tests have to live beside Marionette tests in the tree.

Currently the tests live in /testing/firefox-ui/tests/. Now my goal is to move them to a more visible location, so that also "mach test" can take care of running them in the case of code changes for a specific component, eg. safebrowsing, sessionstore. This includes that also devs would have to contribute enhancements or fixing test failures. 

At the moment there are two open questions which I would need have answered and which may block a final decision for the test location:

1. Can we keep the name firefox-ui for now? It might be a bit misleading given that also other harnesses (mochitest, browser-chrome) run ui tests. But right now folders are mainly named based on the test harness. If the name is not ok, we would have to find a new one, but it would mean lots of changes all over the place (mozilla-central, taskcluster, mozharness, mozmill-ci). If a rename is necessary we would have to go that hard way.

2. Given that we use UI elements for interaction, the tests strongly dependent on the in browser/toolkit defined ui. In case of ui changes the tests can start to fail. So having the tests closely located to the XUL files would be possible. On the other side we have various areas we test (eg. safebrowsing, privatebrowsing, update tests), and IMHO it would make more sense to move them closely to the code they actually test instead of keeping them all together in a single folder under browser/toolkit.

For now I have the following proposal for test file locations.

Browser related tests:
* locationbar: browser/base/content/test/urlbar/firefox-ui
* privatebrowsing: browser/components/privatebrowsing/test/firefox-ui
* safebrowsing: browser/components/safebrowsing/ (unclear because there are no tests)
* sessionstore: browser/components/sessionstore/test/firefox-ui

Toolkit related tests:
* update: toolkit/mozapps/update/test/firefox-ui (mixture of ui which included browser (about dialog) and toolkit (update wizard) ui)

Unit tests:
* Puppeteer: testing/puppeteer/firefox/tests

Dave and Dave, I would kinda appreciate if you can give me feedback regarding the proposed test locations. If you think we should defer the response to a peer please let me know, so that I can extend the ni? appropriately. Thanks!

Comment 18

[Dave Townsend [:mossop]]

Thanks Henrik for the summary.

(In reply to Henrik Skupin (:whimboo) from comment #17)
> 1. Can we keep the name firefox-ui for now? It might be a bit misleading
> given that also other harnesses (mochitest, browser-chrome) run ui tests.
> But right now folders are mainly named based on the test harness. If the
> name is not ok, we would have to find a new one, but it would mean lots of
> changes all over the place (mozilla-central, taskcluster, mozharness,
> mozmill-ci). If a rename is necessary we would have to go that hard way.

What sorts of changes are we talking about here? Presumably you have to make a lot of changes just for moving the tests around or are these test suites already up and running? I'd just like to understand the work involved before making a call here since my preference would be a more ambiguous name like MattN suggested with puppeteer. As he says the amount of confusion it could cause between browser-chrome and other tests is high.

> 2. Given that we use UI elements for interaction, the tests strongly
> dependent on the in browser/toolkit defined ui. In case of ui changes the
> tests can start to fail. So having the tests closely located to the XUL
> files would be possible. On the other side we have various areas we test
> (eg. safebrowsing, privatebrowsing, update tests), and IMHO it would make
> more sense to move them closely to the code they actually test instead of
> keeping them all together in a single folder under browser/toolkit.

Yes this makes sense to me and I agree again with MattN that keeping these segregated in their own directories as we do for other suites is probably the right approach. Your proposed locations look fine.

Comment 19

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

(In reply to Dave Townsend [:mossop] from comment #18)
> (In reply to Henrik Skupin (:whimboo) from comment #17)
> > 1. Can we keep the name firefox-ui for now? It might be a bit misleading
> > given that also other harnesses (mochitest, browser-chrome) run ui tests.
> > But right now folders are mainly named based on the test harness. If the
> > name is not ok, we would have to find a new one, but it would mean lots of
> > changes all over the place (mozilla-central, taskcluster, mozharness,
> > mozmill-ci). If a rename is necessary we would have to go that hard way.
> 
> What sorts of changes are we talking about here? Presumably you have to make
> a lot of changes just for moving the tests around or are these test suites
> already up and running? I'd just like to understand the work involved before
> making a call here since my preference would be a more ambiguous name like
> MattN suggested with puppeteer. As he says the amount of confusion it could
> cause between browser-chrome and other tests is high.

Our tests are located in the tree for a while now. They are present under testing/firefox-ui and testing/puppeteer/firefox. They got moved in earlier this year to avoid the hassle with external repositories. And as part of the tree we run them via Taskcluster on Linux64 for each checkin, and via mozmill-ci for nightly and release builds. The latter happens via the test packages and the appropriate mozharness scripts.

If we decide for another name of the test folders, maybe we can do the remaining changes step by step. Means we could still keep the name of the harness, so it can be run via taskcluster and mozmill-ci and reports as Fxfn/Fxup to Treeherder. Then we could change the harness name, update the mozharness scripts and finally mozmill-ci, which will be the hardest part.

> > 2. Given that we use UI elements for interaction, the tests strongly
> > dependent on the in browser/toolkit defined ui. In case of ui changes the
> > tests can start to fail. So having the tests closely located to the XUL
> > files would be possible. On the other side we have various areas we test
> > (eg. safebrowsing, privatebrowsing, update tests), and IMHO it would make
> > more sense to move them closely to the code they actually test instead of
> > keeping them all together in a single folder under browser/toolkit.
> 
> Yes this makes sense to me and I agree again with MattN that keeping these
> segregated in their own directories as we do for other suites is probably
> the right approach. Your proposed locations look fine.

Thanks, and is there someone else I would have to ask? Or would be feedback from you and Dave Camp be enough? I just want to make sure to not step over anyone's toes.

Comment 20

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> (In reply to Dave Townsend [:mossop] from comment #18)
> > (In reply to Henrik Skupin (:whimboo) from comment #17)
> > > 1. Can we keep the name firefox-ui for now? It might be a bit misleading
> > > given that also other harnesses (mochitest, browser-chrome) run ui tests.
> > > But right now folders are mainly named based on the test harness. If the
> > > name is not ok, we would have to find a new one, but it would mean lots of
> > > changes all over the place (mozilla-central, taskcluster, mozharness,
> > > mozmill-ci). If a rename is necessary we would have to go that hard way.
> > 
> > What sorts of changes are we talking about here? Presumably you have to make
> > a lot of changes just for moving the tests around or are these test suites
> > already up and running? I'd just like to understand the work involved before
> > making a call here since my preference would be a more ambiguous name like
> > MattN suggested with puppeteer. As he says the amount of confusion it could
> > cause between browser-chrome and other tests is high.
> 
> Our tests are located in the tree for a while now. They are present under
> testing/firefox-ui and testing/puppeteer/firefox. They got moved in earlier
> this year to avoid the hassle with external repositories. And as part of the
> tree we run them via Taskcluster on Linux64 for each checkin, and via
> mozmill-ci for nightly and release builds. The latter happens via the test
> packages and the appropriate mozharness scripts.
> 
> If we decide for another name of the test folders, maybe we can do the
> remaining changes step by step. Means we could still keep the name of the
> harness, so it can be run via taskcluster and mozmill-ci and reports as
> Fxfn/Fxup to Treeherder. Then we could change the harness name, update the
> mozharness scripts and finally mozmill-ci, which will be the hardest part.

Sounds like a good plan to me.

> > > 2. Given that we use UI elements for interaction, the tests strongly
> > > dependent on the in browser/toolkit defined ui. In case of ui changes the
> > > tests can start to fail. So having the tests closely located to the XUL
> > > files would be possible. On the other side we have various areas we test
> > > (eg. safebrowsing, privatebrowsing, update tests), and IMHO it would make
> > > more sense to move them closely to the code they actually test instead of
> > > keeping them all together in a single folder under browser/toolkit.
> > 
> > Yes this makes sense to me and I agree again with MattN that keeping these
> > segregated in their own directories as we do for other suites is probably
> > the right approach. Your proposed locations look fine.
> 
> Thanks, and is there someone else I would have to ask? Or would be feedback
> from you and Dave Camp be enough? I just want to make sure to not step over
> anyone's toes.

I think there has been enough discussion here to move forwards assuming dcamp agrees with me.

Comment 21

[Dave Townsend [:mossop]]

I chatted with dcamp briefly and he is happy to just defer to me here.

Comment 22

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

(In reply to Dave Townsend [:mossop] from comment #21)
> I chatted with dcamp briefly and he is happy to just defer to me here.

That's great to hear! So let me bring us back to the naming situation for the tests... As I have heard multiple times now the name "firefox-ui" is not wanted. As an alternative solution "puppeteer" got proposed. I was thinking more over the weekend and I don't feel well with this name due to the following reasons:

1. It is the name of a page object model which we currently make use of. But its use is optional for firefox-ui tests, we don't necessarily depend on it.

2. In the hopefully not too far future we want to switch to use Selenium for our tests and not Marionette. With such a switch the page object model will change most likely to Foxpuppet (https://github.com/mozilla/foxpuppet). With that the name puppeteer would become invalid.

I wonder if we should try to find an artificial name which isn't related to the product we test, nor any library in use.

Dave, what do you think? If you agree, and maybe you have a name in mind I would be happy to hear about. Otherwise we would need to discuss it with the team to find a good name.

Comment 23

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) [away 09/30 - 10/06] from comment #22)
> (In reply to Dave Townsend [:mossop] from comment #21)
> > I chatted with dcamp briefly and he is happy to just defer to me here.
> 
> That's great to hear! So let me bring us back to the naming situation for
> the tests... As I have heard multiple times now the name "firefox-ui" is not
> wanted. As an alternative solution "puppeteer" got proposed. I was thinking
> more over the weekend and I don't feel well with this name due to the
> following reasons:
> 
> 1. It is the name of a page object model which we currently make use of. But
> its use is optional for firefox-ui tests, we don't necessarily depend on it.
> 
> 2. In the hopefully not too far future we want to switch to use Selenium for
> our tests and not Marionette. With such a switch the page object model will
> change most likely to Foxpuppet (https://github.com/mozilla/foxpuppet). With
> that the name puppeteer would become invalid.
> 
> I wonder if we should try to find an artificial name which isn't related to
> the product we test, nor any library in use.
> 
> Dave, what do you think? If you agree, and maybe you have a name in mind I
> would be happy to hear about. Otherwise we would need to discuss it with the
> team to find a good name.

Sorry, I missed this in my bugmail somehow. I don't have a good name suggestion for you unfortunately so I suggest chatting it over with your team.

Comment 24

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

(In reply to Dave Townsend [:mossop] from comment #23)
> Sorry, I missed this in my bugmail somehow. I don't have a good name
> suggestion for you unfortunately so I suggest chatting it over with your
> team.

I will do. While this is on-going I have another question which came up via bug 1304004. Dao has been mentioned that we would need review from a browser peer in terms of how the tests have been written. I really want to have a good quality in our tests, so the question is if a possible refactoring of tests including the code reviews would have to be done before the move.

Comment 25

[Dave Townsend [:mossop]]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> (In reply to Dave Townsend [:mossop] from comment #23)
> > Sorry, I missed this in my bugmail somehow. I don't have a good name
> > suggestion for you unfortunately so I suggest chatting it over with your
> > team.
> 
> I will do. While this is on-going I have another question which came up via
> bug 1304004. Dao has been mentioned that we would need review from a browser
> peer in terms of how the tests have been written. I really want to have a
> good quality in our tests, so the question is if a possible refactoring of
> tests including the code reviews would have to be done before the move.

Do these tests already have review from a competent reviewer? In which case I don't necessarily agree that we'd need to do another review here. Either way I don't think it matters much whether that happens before or after the move, the tests are already running anyway.

Comment 26

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

The tests as existent right now are nearly completely reviewed by myself, or if I wrote patches by Maja or since recently by a peer of the appropriate component. It means that in most cases we didn't have a browser/toolkit peer review.

Comment 27

[Dave Townsend [:mossop]]

Dão, since you brought this up do you have particular concerns over the quality of these tests?

Comment 28

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #27)
> Dão, since you brought this up do you have particular concerns over the
> quality of these tests?

Couple of points:

- The intent of the tests isn't clear to me. What kind of things do they focus on and why? For instance, the tests I had to deal with in bug 1304004 seem to be already covered at least in part by browser-chrome tests, so it's not clear to me why we should have theses tests, and why they should be firefox-ui rather than browser-chrome tests.

- What's these tests' track record for catching real regressions vs. causing maintenance overhead? I mostly came across these tests when they broke because the UI structure changed. Could have been bad luck on my side, I don't know.

- They weren't reviewed by the relevant module peers, i.e. the experts who know what kind of tests we need. No disrespect to Henrik, but this is basically unreviewed code which makes me further pessimistic about its usefulness and quality.

Comment 29

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

(In reply to Dão Gottwald [:dao] from comment #28)
> - The intent of the tests isn't clear to me. What kind of things do they
> focus on and why? For instance, the tests I had to deal with in bug 1304004
> seem to be already covered at least in part by browser-chrome tests, so it's
> not clear to me why we should have theses tests, and why they should be
> firefox-ui rather than browser-chrome tests.

We had similar questions already in the past and I replied to those not once. But well, let me iterate again:

* Our tests purely operate with the elements and do not exercise any available Javascript object like mochitests are doing a lot.

* Our tests are getting run in localized builds of Firefox. Accessing UI elements sometimes happen via their DTD entities and properties. This is not something mochitests can do.

* Our tests can work with remote content like the security tests you refer above. There is mozqa.com which hosts various kinds of certificates and other data for testing. Eg. I talked to David Keeler about 2-3 weeks ago, and he agreed that those tests are helpful and cannot be covered somewhere else. Especially not the EV tests.

* Our tests are able to do a real restart of Firefox, which we use for eg. sessionstore testing. This cannot be covered by mochitest.

> - What's these tests' track record for catching real regressions vs. causing
> maintenance overhead? I mostly came across these tests when they broke
> because the UI structure changed. Could have been bad luck on my side, I
> don't know.

Test failures are getting filed in Testing / Firefox UI Tests. So you indeed haven't seen lots of the other real bugs we filed because our tests caught crashes or other regressions. Given their nature of working with real elements, I know that there is some risk of breakage when the UI is changing. I'm happy to work out a solution to make them more stable if possible. But therefore I would need feedback like on bug 1304004.

> - They weren't reviewed by the relevant module peers, i.e. the experts who
> know what kind of tests we need. No disrespect to Henrik, but this is
> basically unreviewed code which makes me further pessimistic about its
> usefulness and quality.

I talked to several peers already like David Keeler, Marco Bonardo and got their feedback. Reviews for latest safebrowsing tests changes I got from Francois. Also people like Mike Conley, and Robert Strong contributed to our tests. The above doesn't span all of our tests yet, but so far we got positive feedback.


Anyway, we had a further discussion about such a move of the tests, and given the barriers we see here also with naming, I proposed an alternative solution which we might want to finally do. This included getting Firefox Puppeteer decoupled from Firefox UI tests, and get the tests written as purely Marionette tests with the possibility to optionally use Firefox Puppeteer. With that we would be able to stick all the tests in the appropriate tests/marionette subfolders. Exception would be the update tests, which would have to be kept separately. 

The next days I will hash out the strategy of refactoring those tests, and will file a new tracking bug. This bug most likely becomes a wontfix then.

Comment 30

[Dave Townsend [:mossop]]

(In reply to Dão Gottwald [:dao] from comment #28)
> (In reply to Dave Townsend [:mossop] from comment #27)
> > Dão, since you brought this up do you have particular concerns over the
> > quality of these tests?
> 
> Couple of points:
> 
> - The intent of the tests isn't clear to me. What kind of things do they
> focus on and why? For instance, the tests I had to deal with in bug 1304004
> seem to be already covered at least in part by browser-chrome tests, so it's
> not clear to me why we should have theses tests, and why they should be
> firefox-ui rather than browser-chrome tests.

Whether we should have them or not is not really at question here since we already have them. More testing is good.

> - What's these tests' track record for catching real regressions vs. causing
> maintenance overhead? I mostly came across these tests when they broke
> because the UI structure changed. Could have been bad luck on my side, I
> don't know.

A valid question but again not relevant here.

> - They weren't reviewed by the relevant module peers, i.e. the experts who
> know what kind of tests we need. No disrespect to Henrik, but this is
> basically unreviewed code which makes me further pessimistic about its
> usefulness and quality.

So it sounds like your only concern is not the quality of the test code itself but whether the tests are testing something useful. That sounds like a much easier review for a module owner to make and one that can easily happen at a later point.

Henrik, after moving all the tests it might make sense to email the relevant owners of the code under test to make sure they are aware of the new tests and then they can use their judgement in deciding whether anything additional needs to be done.

Comment 31

[Dão Gottwald [:dao]]

(In reply to Henrik Skupin (:whimboo) from comment #29)
> * Our tests purely operate with the elements and do not exercise any
> available Javascript object like mochitests are doing a lot.

Why is that a benefit? It sounds like a rather annoying limitation.

> * Our tests are getting run in localized builds of Firefox. Accessing UI
> elements sometimes happen via their DTD entities and properties. This is not
> something mochitests can do.

I don't understand why accessing elements via DTD entities is beneficial, or why mochitests couldn't run in non-en-US builds if we wanted that.

> * Our tests can work with remote content like the security tests you refer
> above. There is mozqa.com which hosts various kinds of certificates and
> other data for testing. Eg. I talked to David Keeler about 2-3 weeks ago,
> and he agreed that those tests are helpful and cannot be covered somewhere
> else. Especially not the EV tests.

Good to know, although I would point out that it was a deliberate choice to block automated tests from accessing remote content.

> * Our tests are able to do a real restart of Firefox, which we use for eg.
> sessionstore testing. This cannot be covered by mochitest.

Handy for sessionstore tests indeed, I imagine.

Comment 32

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #30)
> (In reply to Dão Gottwald [:dao] from comment #28)
> > (In reply to Dave Townsend [:mossop] from comment #27)
> > > Dão, since you brought this up do you have particular concerns over the
> > > quality of these tests?
> > 
> > Couple of points:
> > 
> > - The intent of the tests isn't clear to me. What kind of things do they
> > focus on and why? For instance, the tests I had to deal with in bug 1304004
> > seem to be already covered at least in part by browser-chrome tests, so it's
> > not clear to me why we should have theses tests, and why they should be
> > firefox-ui rather than browser-chrome tests.
> 
> Whether we should have them or not is not really at question here since we
> already have them. More testing is good.

"More testing is good" seems a bit over-simplistic to me. Somewhere between "test needs no maintenance and is likely to catch regressions" and "test needs constant maintenance and probably won't ever catch a regression" we need to draw a line, otherwise we spend all our time fixing tests and get no work done.

Comment 33

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

We decided against getting this bug fixed. Mainly because of the amount of work to be done. Instead I filed bug 1312359 which will port the important fx-ui tests to plain Marionette tests. This will solve the naming issue for the tests, and causes lesser work on dependent external systems.