Bug 112890 - NRWT: --force should run tests as if they're expected to pass
Summary: NRWT: --force should run tests as if they're expected to pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 123167
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-21 02:26 PDT by Ryosuke Niwa
Modified: 2013-10-31 11:11 PDT (History)
14 users (show)

See Also:


Attachments
Modifies the behaviour of nrwt --force switch. (5.35 KB, patch)
2013-10-21 07:07 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff
Modifies the behaviour of nrwt --force switch. (5.35 KB, patch)
2013-10-31 04:13 PDT, Tamas Gergely
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-03-21 02:26:04 PDT
--force appears to be broken at least on Mac WK1.
Comment 1 Eric Seidel (no email) 2013-03-21 02:38:21 PDT
    --force             Run all tests, even those marked SKIP in the test list (same as --skipped=ignore)
Comment 2 Ryosuke Niwa 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.
Comment 3 Dirk Pranke 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.
Comment 4 Dirk Pranke 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?
Comment 5 Ryosuke Niwa 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.
Comment 6 Dirk Pranke 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Ryosuke Niwa 2013-03-21 14:02:16 PDT
Ugh… I guess ignoring skips might sound ambiguous. Maybe --run-skipped?
Comment 9 Dirk Pranke 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.
Comment 10 Ryosuke Niwa 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.
Comment 11 Tamas Gergely 2013-10-21 07:07:19 PDT
Created attachment 214736 [details]
Modifies the behaviour of nrwt --force switch.
Comment 12 Dirk Pranke 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.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2013-10-21 19:16:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Csaba Osztrogonác 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
Comment 17 Tamas Gergely 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
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2013-10-31 11:11:27 PDT
All reviewed patches have been landed.  Closing bug.