Bug 112890 – NRWT: --force should run tests as if they're expected to pass

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

--force appears to be broken at least on Mac WK1.

[Eric Seidel (no email)]

    --force             Run all tests, even those marked SKIP in the test list (same as --skipped=ignore)

[Ryosuke Niwa]

(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]

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]

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]

(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]

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]

(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]

Ugh… I guess ignoring skips might sound ambiguous. Maybe --run-skipped?

[Dirk Pranke]

--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]

(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]

Created attachment 214736 [details]
Modifies the behaviour of nrwt --force switch.

[Dirk Pranke]

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 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]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

This caused 2 python tests to fail on all platforms:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20WK2%20%28Tests%29/builds/12800/steps/webkitpy-test/logs/stdio

[Csaba Osztrogonác]

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]

Created attachment 215642 [details]
Modifies the behaviour of nrwt --force switch.

Corrected, now --force really implies --skipped=ignore

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.