Closed Bug 1429580 Opened 7 years ago Closed 7 years ago

skip-if(some_condition) include some_folder/reftest.list behaves unexpectedly

Categories

(Testing :: Reftest, enhancement)

Product:
Testing
Component:
Reftest
Version:
59 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

Bug 1429580 - Change the semantics of skipping reftest includes.
(deleted), text/x-review-board-request
dbaron
: review+
Details
Bug 1429580 - Remove now-unnecessary inherited_status argument.
(deleted), text/x-review-board-request
dbaron
: review+
Details
See dev-platform thread at https://groups.google.com/forum/#!topic/mozilla.dev.platform/sWbUNO33yYc for details. In summary, doing something like [1] behaves counterintuitively in that it still runs fuzzy tests inside the included reftest.list file, instead of just skipping the entire reftest.list file. We should change the semantics of this construct to behave more intuitively. [1] https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/layout/reftests/reftest.list#275
That was looking green enough so I cleaned up the patch a bit more. Here's a new try push (I cancelled the remaining jobs on the last one) to make sure I didn't accidentally break anything: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b36d849aa7c1f387d6adbb0c465c25620eaa282e
Comment on attachment 8941626 [details] Bug 1429580 - Change the semantics of skipping reftest includes. https://reviewboard.mozilla.org/r/211876/#review217694 Thanks for cleaning this up. I think this is mostly reasonable, except I'm marking it as review- because I think the code still allows too many syntactic options that it shouldn't (and that the documentation says aren't allowed). In particular: - it allows "fails-if(false)" because the condition is never true - it allows "fails skip" because the skip overrides the fails I think you should do something like adding a boolean that tracks whether a non-skip status was ever used, and reject any include line where that boolean was set. Second (and unrelated to the review-), this patch allows for some code removal that should be in a second patch. In particular, it makes the inherited_status parameter to ReadManifest unused (since it is now always EXPECTED_PASS). So you should write a second patch to remove inherited_status.
Attachment #8941626 - Flags: review?(dbaron) → review-
Comment on attachment 8941626 [details] Bug 1429580 - Change the semantics of skipping reftest includes. https://reviewboard.mozilla.org/r/211876/#review217892 r=dbaron with this change ::: layout/tools/reftest/manifest.jsm:149 (Diff revision 2) > } else if (item == "needs-focus") { > needs_focus = true; > cond = false; Please also set nonSkipUsed to true here, and in every "else if" following (but not the else at the end).
Attachment #8941626 - Flags: review?(dbaron) → review+
Comment on attachment 8941626 [details] Bug 1429580 - Change the semantics of skipping reftest includes. https://reviewboard.mozilla.org/r/211876/#review217894 ::: layout/tools/reftest/manifest.jsm:149 (Diff revision 2) > } else if (item == "needs-focus") { > needs_focus = true; > cond = false; Er, ignore this; what you have works fine.
Comment on attachment 8941834 [details] Bug 1429580 - Remove now-unnecessary inherited_status argument. https://reviewboard.mozilla.org/r/212068/#review217896
Attachment #8941834 - Flags: review?(dbaron) → review+
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c177a92b17cc Change the semantics of skipping reftest includes. r=dbaron https://hg.mozilla.org/integration/autoland/rev/1250522ac712 Remove now-unnecessary inherited_status argument. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/c177a92b17cc https://hg.mozilla.org/mozilla-central/rev/1250522ac712
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
As I happen to have found a problem in a printing-related preprocessor instruction in manifest.jsm, and was looking to run those tests, I couldn't figure out in what chunk they were running. I then looked back in time, and found it used to run in OSX debug R2... and that it stopped running after this bug. Is that expected? Last R2 log before this landed: https://queue.taskcluster.net/v1/task/Dtl7gJn-RnO1VQxyjd40Uw/runs/0/artifacts/public/logs/live_backing.log First R2 log after this landed: https://queue.taskcluster.net/v1/task/Gb65SbBARwK0SwqNwR-Bew/runs/0/artifacts/public/logs/live_backing.log (and I checked, they didn't move to R1)
Flags: needinfo?(bugmail)
Which tests are you asking about? It sounds like printing tests, but it's not clear. Assuming it's printing tests, yes it is expected. The printing tests are skipped if !skiaPdf is true [1], which it is on OS X (you can see that in the reftests sandbox dump in the logs you quoted). Even in the first log the tests were being skipped, in the second one they are skipped but don't show up at all in the log because the entire folder of reftests is skipped. So functionally there's no difference. [1] https://searchfox.org/mozilla-central/rev/f41017f5cd7b5f3835d282fbe011d2899574fc2b/layout/reftests/reftest.list#300
Flags: needinfo?(bugmail)
Thanks for the clarification. FWIW, skiaPdf is intended to be true on osx, but the #ifdef that does that is wrong, and that's what I was changing and trying to figure out what to run to run those tests. (see https://reviewboard.mozilla.org/r/217074/diff/1 )
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: