Bug 105219 - Running a skipped test with run-perf-tests could alert the user
Summary: Running a skipped test with run-perf-tests could alert the user
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: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 77037
  Show dependency treegraph
 
Reported: 2012-12-17 14:58 PST by Philip Rogers
Modified: 2012-12-18 13:16 PST (History)
5 users (show)

See Also:


Attachments
Fixes the bug (5.63 KB, patch)
2012-12-18 00:06 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fix a minor typo (5.63 KB, patch)
2012-12-18 00:08 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-12-17 14:58:23 PST
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.
Comment 1 Ryosuke Niwa 2012-12-17 15:06:56 PST
(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?
Comment 2 Philip Rogers 2012-12-17 15:20:21 PST
(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.
Comment 3 Eric Seidel (no email) 2012-12-17 15:22:47 PST
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.
Comment 4 Eric Seidel (no email) 2012-12-17 15:23:37 PST
This isn't a critical bug, just something we should be aware of. :)
Comment 5 Ryosuke Niwa 2012-12-18 00:06:50 PST
Created attachment 179892 [details]
Fixes the bug
Comment 6 Ryosuke Niwa 2012-12-18 00:08:44 PST
Created attachment 179894 [details]
Fix a minor typo
Comment 7 Eric Seidel (no email) 2012-12-18 10:52:01 PST
Comment on attachment 179894 [details]
Fix a minor typo

Lgtm
Comment 8 WebKit Review Bot 2012-12-18 11:16:29 PST
Comment on attachment 179894 [details]
Fix a minor typo

Clearing flags on attachment: 179894

Committed r138045: <http://trac.webkit.org/changeset/138045>
Comment 9 WebKit Review Bot 2012-12-18 11:16:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Dirk Pranke 2012-12-18 11:33:07 PST
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?
Comment 11 Ryosuke Niwa 2012-12-18 11:36:37 PST
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.
Comment 12 Ryosuke Niwa 2012-12-18 13:08:25 PST
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.
Comment 13 Dirk Pranke 2012-12-18 13:12:19 PST
(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?
Comment 14 Ryosuke Niwa 2012-12-18 13:16:18 PST
(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 :(