Closed Bug 1698591 Opened 3 years ago Closed 2 years ago

Harmonize test fail/skip annotations between upstream Puppeteer and mozilla-central

Categories

(Remote Protocol :: CDP, task, P3)

Product:
Remote Protocol
Component:
CDP
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: jdescottes, Unassigned)

References

Details

In Puppeteer upstream repository, two helpers are used in the test suite to skip tests in Firefox:

  • describeFailsFirefox
  • itFailsFirefox

Those helpers will completely skip the corresponding describe/it when running in Firefox.
In mozilla central, we decided to take a different approach and to run the tests, but we expect a failure, which is described in a manifest puppeteer-expected.json: https://searchfox.org/mozilla-central/source/remote/test/puppeteer-expected.json

It makes the vendoring process complex because after updating puppeteer, you need to manually replace all the describeFailsFirefox/itFailsFirefox by describe/it. The issue is that we still have some describeFailsFirefox/itFailsFirefox in tree: https://searchfox.org/mozilla-central/search?q=%28it%7Cdescribe%29FailsFirefox%5C%28&path=&case=false&regexp=true

This means you must be careful to only update the describeFailsFirefox/itFailsFirefox which were not already in tree.

The reason why we have different approaches is that in mozilla-central we want to keep running the test and be notified in case we happen to fix something rather than leaving tests skipped forever. But upstream puppeteer prefers to completely skip the test (might also be due to limitation of the test harness used there).

It would be great if we could reduce the code changes between our puppeteer version and the upstream version. One suggestion could be to keep the test code identical, and instead only apply a patch to the helpers describeFailsFirefox/itFailsFirefox to map them to describe/it. This would be much easier to maintain.

To do that however we'll need to check the reason why we have kept some describeFailsFirefox/itFailsFirefox in tree. If it's intended and we really want to skip those tests in mozilla-central then we'll need a slight API update to express the difference between failing tests and skipped tests in code. For instance we could propose to upstream puppeteer to have 2 sets of helpers:

  • describeFailsFirefox/itFailsFirefox
  • describeSkipsFirefox/itSkipsFirefox

The implementation of the 2 helpers would be the same upstream, they would skip the test completely, but in mozilla-central we could modify the "fail" version to still run the test. We would still mark the test as expected to FAIL via the manifest.

(In reply to Julian Descottes [:jdescottes] from comment #0)

It would be great if we code reduce the code changes between our puppeteer version and the upstream version. One suggestion could be to keep the test code identical, and instead only apply a patch to the helpers describeFailsFirefox/itFailsFirefox to map them to describe/it. This would be much easier to maintain.

To do that however we'll need to check the reason why we have kept some describeFailsFirefox/itFailsFirefox in tree. If it's intended and we really want to skip those tests in mozilla-central then we'll need a slight API update to express the difference between failing tests and skipped tests in code. For instance we could propose to upstream puppeteer to have 2 sets of helpers:

  • describeFailsFirefox/itFailsFirefox
  • describeSkipsFirefox/itSkipsFirefox

The implementation of the 2 helpers would be the same upstream, they would skip the test completely, but in mozilla-central we could modify the "fail" version to still run the test. We would still mark the test as expected to FAIL via the manifest.

Hi Henrik, we discussed this topic during the last meeting. How do you feel about the suggestion above, introducing an "itSkipsOnFirefox" helper upstream to help distinguish tests that should be skipped from tests that should fail. This way we would no longer have to manually update any test after a sync. Let me know what you think about the proposal, I can propose a patch upstream if you like the idea.

Flags: needinfo?(hskupin)

Having lesser work to do for syncs whether it's upstream or downstream is definitely a good idea! So I clearly second this suggestion, and if you could bring up this topic on the Puppeteer repository we all could figure out what the best naming would be.

Benefit of that would also be that on Puppeteers side it's clear which tests are out of scope for Firefox.

Flags: needinfo?(hskupin)

Actually isn't there a flag to only run a test with Chrome? I think that such a flag would more appropriate instead of having a skip one.

Thanks for the feedback. And good point, there is already itChromeOnly/describeChromeOnly.

So we could consider that itFailsFirefox/describeFailsFirefox is supposed to run on Firefox but will fail, and itChromeOnly/describeChromeOnly should be skipped on Firefox. In practice, I don't know if there is a difference between isFirefox and !isChrome. Are there any other browsers supported? It feels a bit weird to have one API relying on isChrome and the other on isFirefox, but nothing that can't be worked out.

Assuming we can use itChromeOnly/describeChromeOnly, it means we would just update the few tests which are still marked as itFailsFirefox/describeFailsFirefox in mozilla-central to itChromeOnly/describeChromeOnly, synchronize that upstream. Then we would do a sync, and just update our local copy of itFailsFirefox/describeFailsFirefox. I can already provide patches here to show how it would look like and I'll open an issue on the puppeteer repo to follow up.

(In reply to Julian Descottes [:jdescottes] from comment #4)

So we could consider that itFailsFirefox/describeFailsFirefox is supposed to run on Firefox but will fail, and itChromeOnly/describeChromeOnly should be skipped on Firefox. In practice, I don't know if there is a difference between isFirefox and !isChrome. Are there any other browsers supported? It feels a bit weird to have one API relying on isChrome and the other on isFirefox, but nothing that can't be worked out.

No, it's just Chrome and Firefox. Apple will not add support for CDP to Safari. So this set of checks should be totally fine.

Assuming we can use itChromeOnly/describeChromeOnly, it means we would just update the few tests which are still marked as itFailsFirefox/describeFailsFirefox in mozilla-central to itChromeOnly/describeChromeOnly, synchronize that upstream. Then we would do a sync, and just update our local copy of itFailsFirefox/describeFailsFirefox. I can already provide patches here to show how it would look like and I'll open an issue on the puppeteer repo to follow up.

I think that this sounds fine. Once all this has been done we should still keep an eye out for new tests that get added, which might fall into that category of Chrome only again. But that should be easy to do for any downstream sync. Mind adding a new section to the sync documentation along with the changes?

Also I don't think we need to downstream sync right away again. We can wait once there are enough changes upstream for a Puppeteer release.

Thanks Julian!

I wonder if this bug is still needed now that we have the new expectation file.

Component: Agent → CDP

Right, I don't think we need this bug anymore. Also the two helpers are gone now (from our codebase and from puppeteer in general). We should update our documentation which still mentions this: https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/remote/test/puppeteer/test/README.md#26-33

I will file another bug for updating the doc, unless there is already one.
Let's close this one. Closing as FIXED and adding Bug 1797723 as See also, feel free to update.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
See Also: → 1797723

There is no fix that has been landed. With the underlying issue gone the bug should actually be invalid now. Thanks for checking!

Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.