Bug 105219 – Running a skipped test with run-perf-tests could alert the user

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

[Philip Rogers]

Today I ran a skipped perf test like so:
run-perf-tests [skipped test]
Running 0 tests

It would be great if this ran the skipped test anyway, or alerted the user that the test would not be run.

[Ryosuke Niwa]

(In reply to comment #0)
> Today I ran a skipped perf test like so:
> run-perf-tests [skipped test]
> Running 0 tests
> 
> It would be great if this ran the skipped test anyway, or alerted the user that the test would not be run.

Use --force?

[Philip Rogers]

(In reply to comment #1)
> (In reply to comment #0)
> > Today I ran a skipped perf test like so:
> > run-perf-tests [skipped test]
> > Running 0 tests
> > 
> > It would be great if this ran the skipped test anyway, or alerted the user that the test would not be run.
> 
> Use --force?

The issue is simply that the user may not realize the test was skipped.

[Eric Seidel (no email)]

Yeah, I'm sure there are ways around it.  It was just very confusing. :)

It gave no info.  He was using:

%run-perf-tests SVG/GearFlowers.html
Running 0 tests

And that's literally all the output. :)

I guess in general we should validate the arguments and explain better to the user what's happening!  If you pass non/existent/path you get the same confusion.

[Eric Seidel (no email)]

This isn't a critical bug, just something we should be aware of. :)

[Ryosuke Niwa]

Created attachment 179892 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 179894 [details]
Fix a minor typo

[Eric Seidel (no email)]

Comment on attachment 179894 [details]
Fix a minor typo

Lgtm

[WebKit Review Bot]

Comment on attachment 179894 [details]
Fix a minor typo

Clearing flags on attachment: 179894

Committed r138045: <http://trac.webkit.org/changeset/138045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

Comment on attachment 179894 [details]
Fix a minor typo

View in context: https://bugs.webkit.org/attachment.cgi?id=179894&action=review

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:158
> +            if self._options.use_skipped_list and self._port.skips_perf_test(relative_path) and filesystem.normpath(path) in paths:

I'm feeling stupid today, so I'm probably missing something, but why do you want the 'filesystem.normpath(path) in paths' part? won't that make you skip things that you listed on the command line?

[Ryosuke Niwa]

Comment on attachment 179894 [details]
Fix a minor typo

View in context: https://bugs.webkit.org/attachment.cgi?id=179894&action=review

>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:158
>> +            if self._options.use_skipped_list and self._port.skips_perf_test(relative_path) and filesystem.normpath(path) in paths:
> 
> I'm feeling stupid today, so I'm probably missing something, but why do you want the 'filesystem.normpath(path) in paths' part? won't that make you skip things that you listed on the command line?

Huh, that seems right. I wonder why none of the tests caught that.

[Ryosuke Niwa]

Fixed in http://trac.webkit.org/changeset/138058. This was a pretty bad regression. It caused all perf. tests regardless of whether they're skipped or not are ran :( I've cancelled all perf bot jobs up until r138058.

[Dirk Pranke]

(In reply to comment #12)
> Fixed in http://trac.webkit.org/changeset/138058. This was a pretty bad regression. It caused all perf. tests regardless of whether they're skipped or not are ran :( I've cancelled all perf bot jobs up until r138058.

I'm a bit confused by that change ... you mean that it caused all the perf tests to be skipped, right? Also, Did changing the unit test name actually affect anything?

[Ryosuke Niwa]

(In reply to comment #13)
> I'm a bit confused by that change ... you mean that it caused all the perf tests to be skipped, right? Also, Did changing the unit test name actually affect anything?

No. It meant that Skipped was completely ignored because the last condition was always false. Renaming the unit test name was necessarily because there was another unit test of the same, which actually tested that skipped list worked.

This particular test was about --foce, so skipped list was ignored there. As a result, we didn't have any unit test for skip list :(