RESOLVED FIXED 112890
NRWT: --force should run tests as if they're expected to pass
https://bugs.webkit.org/show_bug.cgi?id=112890
Summary NRWT: --force should run tests as if they're expected to pass
Ryosuke Niwa
Reported 2013-03-21 02:26:04 PDT
--force appears to be broken at least on Mac WK1.
Attachments
Modifies the behaviour of nrwt --force switch. (5.35 KB, patch)
2013-10-21 07:07 PDT, Tamas Gergely
no flags
Modifies the behaviour of nrwt --force switch. (5.35 KB, patch)
2013-10-31 04:13 PDT, Tamas Gergely
no flags
Eric Seidel (no email)
Comment 1 2013-03-21 02:38:21 PDT
--force Run all tests, even those marked SKIP in the test list (same as --skipped=ignore)
Ryosuke Niwa
Comment 2 2013-03-21 11:05:46 PDT
(In reply to comment #1) > --force Run all tests, even those marked SKIP in the test list (same as --skipped=ignore) Hm... maybe it's doing the right thing? What I would expect --force to do is to treat all tests to have Pass expectation.
Dirk Pranke
Comment 3 2013-03-21 12:06:14 PDT
Yup, that's not what it does (nor is it what it's ever done). You're suggesting a reasonable feature addition, though.
Dirk Pranke
Comment 4 2013-03-21 13:52:36 PDT
To clarify, if a test is marked as Skip (only), then when it is run, it will be expected to pass. You're suggesting that --force should do something else, and also change the expectations for tests that are otherwise normally run but expected to fail one way or another?
Ryosuke Niwa
Comment 5 2013-03-21 13:54:20 PDT
(In reply to comment #4) > To clarify, if a test is marked as Skip (only), then when it is run, it will be expected to pass. > > You're suggesting that --force should do something else, and also change the expectations for tests that are otherwise normally run but expected to fail one way or another? I'm expecting that Failure, ImageOnlyFailure, Crash, etc… test expectations to be ignored, and all tests to be expected to Pass.
Dirk Pranke
Comment 6 2013-03-21 13:55:39 PDT
Ok. I'm not opposed to this idea, but I'm not sure if we should use --force for that. It might confuse people used to the existing behavior (or break scripts using it). On the other hand, it does seem like a reasonable name, and we do have --skipped= for the other behavior.
Ryosuke Niwa
Comment 7 2013-03-21 14:01:46 PDT
(In reply to comment #6) > Ok. I'm not opposed to this idea, but I'm not sure if we should use --force for that. It might confuse people used to the existing behavior (or break scripts using it). > > On the other hand, it does seem like a reasonable name, and we do have --skipped= for the other behavior. Do you know if there are any tools that depend on this behavior? I feel like the current --force should be renamed to --ignore-skips.
Ryosuke Niwa
Comment 8 2013-03-21 14:02:16 PDT
Ugh… I guess ignoring skips might sound ambiguous. Maybe --run-skipped?
Dirk Pranke
Comment 9 2013-03-21 14:09:20 PDT
--force is currently a synonym for --skipped=ignore . If we can get people to change at all, they should just change to that, rather than adding yet another --run-skipped or --ignore-skipped flag. I do not know if any scripts currently use this. I think Ossy has done something along these lines in the past, so I'm cc'ing him.
Ryosuke Niwa
Comment 10 2013-03-21 14:12:23 PDT
(In reply to comment #9) > --force is currently a synonym for --skipped=ignore . If we can get people to change at all, they should just change to that, rather than adding yet another --run-skipped or --ignore-skipped flag. Yeah, I agree. We already have waaay too many options in new-run-webkit-tests.
Tamas Gergely
Comment 11 2013-10-21 07:07:19 PDT
Created attachment 214736 [details] Modifies the behaviour of nrwt --force switch.
Dirk Pranke
Comment 12 2013-10-21 11:42:37 PDT
The code looks correct. However, this patch appears to ignore much of the discussion in the bug ... are you sure that you want to change the semantics of the existing flag? Insofar as I'm not working on the code in WebKit much these days, I'll let someone else (rniwa?) decide if they want to r+ this or not.
WebKit Commit Bot
Comment 13 2013-10-21 19:16:01 PDT
Comment on attachment 214736 [details] Modifies the behaviour of nrwt --force switch. Clearing flags on attachment: 214736 Committed r157774: <http://trac.webkit.org/changeset/157774>
WebKit Commit Bot
Comment 14 2013-10-21 19:16:04 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 16 2013-10-23 01:53:28 PDT
Comment on attachment 214736 [details] Modifies the behaviour of nrwt --force switch. View in context: https://bugs.webkit.org/attachment.cgi?id=214736&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:341 > + if options.force and options.skipped not in ('ignore', 'default'): > + _log.warning("--force overrides --skipped=%s" % (options.skipped)) > + options.skipped = 'ignore' > + It seems the bug is here. After this change --force makes NRWT run [ Failure ] marked tests if they marked as [ Pass ], but it doesn't run [ Skip ] and [ WontFix ] marked tests as they pass. The problem is that options.skipped = 'default' by default, but it should be 'ignore' if options.force is true. Tamás, please run Tools/Scripts/test-webkitpy unit tests next time you touches webkitpy codes. I thought CQ bots run webkitpy tests ... but it seems they don't. I found this: "# FIXME: We should teach the commit-queue and the EWS how to run these tests." in Tools/Scripts/webkitpy/tool/steps/runtests.py
Tamas Gergely
Comment 17 2013-10-31 04:13:21 PDT
Created attachment 215642 [details] Modifies the behaviour of nrwt --force switch. Corrected, now --force really implies --skipped=ignore
WebKit Commit Bot
Comment 18 2013-10-31 11:11:22 PDT
Comment on attachment 215642 [details] Modifies the behaviour of nrwt --force switch. Clearing flags on attachment: 215642 Committed r158373: <http://trac.webkit.org/changeset/158373>
WebKit Commit Bot
Comment 19 2013-10-31 11:11:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.