Details
- Reviewers
jmaher - Commits
- rMOZILLACENTRAL99fec8dd463d: Bug 1930106 - Update skip-if conditions r=jmaher
- Bugzilla Bug ID
- 1930106
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
Event Timeline
Push used to generate the skip-if conditions: https://treeherder.mozilla.org/jobs?repo=try&revision=2187a676564936c7800239dd209915e509184399
./mach manifest skip-fails --b disable --turbo "https://treeherder.mozilla.org/jobs?repo=try&revision=2187a676564936c7800239dd209915e509184399"
| toolkit/content/tests/widgets/chrome.toml | ||
|---|---|---|
| 23 | we don't run this test on linux22.04, so we can remove this | |
| 24 | hand edit this to remove the # Bug TBD, this skipped os == 'mac' previously with no comment. | |
| toolkit/content/tests/widgets/mochitest.toml | ||
| 46 | the linux failures are passing in CI, can you link your try push? possibly this is on unsupported variants? | |
| 47 | here for mac, remove the comment, it existed before without a comment. | |
| 77 | for linux, keep the comment: #TIMED_OUT #Bug 1511256 | |
| 78 | I don't see this failing in CI, can you point to the try push? | |
| 116 | for the linux lines, please add the comment from the original: # bug 1804621 | |
| 117 | same as above, this passes in CI and I need to see why this is skipped now. | |
| toolkit/content/tests/widgets/chrome.toml | ||
|---|---|---|
| 23 | ||
| toolkit/content/tests/widgets/chrome.toml | ||
|---|---|---|
| 23 | Without --full in https://treeherder.mozilla.org/jobs?repo=try&revision=e1bcdd28ffeb8ed8190f1718ac2ffde16f544b2c, the test is indeed not scheduled. But sadly without --full, some test variants are not scheduled and thus the script cannot fully simplify some conditions | |
looking at the push without --full, I see all green, but it looks like tests are skipped either with the default skips or the ones you added. when testing with --full one has to be careful and accept a lot of noise. For the scenarios where we normally don't schedule tests (I think this only applies to 22.04, specifically mochitest-chrome, i.e. the c1 tasks), we shouldn't add conditions. Unfortunately linux ccov can't be seen without --full, but the rest can.
for scenarios where there was an existing skip-if, and now there is a more complete (or multiline skip-if) which covers the same thing, we can use the same comments for the same platforms (if there was no comment, no need to have one now).
In the scenarios where we have new skip-ifs, that is where we need to look in more detail.
Maybe updating this patch by combining it with the removal (first patch in the series), then removing the chrome.toml references for os_version='22.04', finally ensure the comments are the same, and if this is a new skip, leave the bug there, so we can see the scope of the new bugs.
Ideally I would like to remove the new skip-ifs (not the expanded ones) and run on try to see if those fail or not. It is likely that the act of running ALL tests would fail for ones with valid skip-ifs, but when they run it could affect the state of the browser causing other tests to fail. This happens a lot, the tests were probably written a long time ago and haven't been updated to cleanup the state of the browser.
Code analysis found 3 defects in diff 952312:
- 3 defects found by py-black (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 952312.
Try push made against this stack https://treeherder.mozilla.org/jobs?repo=try&revision=ad3ee5dd1372990f7f60c3a72f15f6fe8aeb44f6
Code analysis found 1723 defects in diff 952344:
- 509 defects found by py-ruff (Mozlint)
- 1214 defects found by eslint (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 952344.
overall this is looking better- so far the cleanup is looking really great.
I am thinking if we are skipping most of the conditions it would be nice to add a feature (or a debug line with --verbose) that indicates what it passed on, or what we don't have data for. Possibly we can find a way to add the remaining data to ensure we get as complete as possible.
I don't want to block on perfection, I would rather land and make improvements later next week as we decide upon them.
| toolkit/content/tests/widgets/chrome.toml | ||
|---|---|---|
| 15–16 | if these don't fail anymore, please remove the entire line. Looking at the try push logs, these seem to run just fine. | |
| 88 | same here, remove this blank line | |
| toolkit/content/tests/widgets/mochitest.toml | ||
| 50 | out of curiousity where does this pass? os == 'linux' && os_version == '18.04' && processor == 'x86_64' && opt && !fission ? Ideally a comment could indicate what specific configs this does run and pass on, is seems that most of these are failing and I would wonder what else passes, and if not run on the try push, we should add them to the try push to see if they pass or fail. | |
| toolkit/content/tests/widgets/mochitest.toml | ||
|---|---|---|
| 50 | No test passes on the push, but the artifact at https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2.mozilla-central.latest.source.test-info-all/artifacts/public%2Ftest-info-testrun-matrix.json expects a test using the ccov test variant which is not scheduled unless --full is used. | |
| 77 | That bugzilla ticket is closed and skipfails created a new one 1929419 | |
| toolkit/content/tests/widgets/mochitest.toml | ||
|---|---|---|
| 50 | When using the latest artifact, the issue is resolved, there is no need to use --full | |
| toolkit/content/tests/widgets/mochitest.toml | ||
|---|---|---|
| 46 | on these pushes, I don't see test_image_recognition_unsupported.html failing, only test_videocontrols.html fails. | |
| 47 | in this case I agree with you. In general it is better to carry forward the state as much as possible. But in a world where we have automation doing most of the work, we need uniform output. | |
| 116 | the original comments help with history- we know this existed a long time ago, there might be prior art around investigation, etc. The use case for the script is not to remove old conditions and re apply conditions, only to add to existing conditions. I have future plans that need a lot of work to re-enable tests that pass, that is similar to what you have done here; Obviously it is quirky as there is a need to manually delete conditions and resolve new vs old comments. Some comments are there as tests are skipped because when run they cause other tests to pass or fail. | |
| toolkit/content/tests/widgets/mochitest.toml | ||
|---|---|---|
| 46 | Indeed I'll try to check why | |
Code analysis found 1 defect in diff 954681:
- 1 defect found by py-black (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 954681.
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!