Bug 98260 - [NRWT] --skipped option is ignored when --test-list is used
Summary: [NRWT] --skipped option is ignored when --test-list is used
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Dirk Pranke
URL:
Keywords: NRWT
Depends on:
Blocks:
 
Reported: 2012-10-03 06:29 PDT by Csaba Osztrogonác
Modified: 2012-10-04 03:18 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2012-10-03 20:03 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-10-03 06:29:53 PDT
Now NRWT runs all tests listed in test-list file, --skipped option is 
absolutely ignored. In my opinion it should respect --skipped option too.

This bug is revealed with a real life usecase:
- After a fix I run all tests on WK1: run-webkit-tests
- There are 600+ failing tests. I checked one by one, only rebase needed. (list-of-rebased-tests.txt)
- I did the rebase to platform/qt
- double check for WK2 (maybe some of them need qt-5.0-wk2 specific result):
  run-webkit-tests --test-list=./list-of-rebased-tests.txt --qt -2
--> But unfortunately NRWT runs skipped tests (wk2, qt-5.0-wk2) too, but it shouldn't

Any idea how to fix this annoying bug?
Comment 1 Alexey Proskuryakov 2012-10-03 10:37:09 PDT
FWIW, it's intentional that tests listed on command line are run regardless of being skipped. Test-list file is similar conceptually, although use cases are clearly different.
Comment 2 Dirk Pranke 2012-10-03 11:54:24 PDT
Alexey is basically right on the implementation and original intent, but that doesn't mean we shouldn't make your case work.

To make sure that I understand what you're seeing / asking for, you have a list of tests in a file, and some of those tests are listed as [ Skip ] in TestExpectations, but when you run NRWT, they get executed, right? 

So, you'd actually like the expectations to be honored and the files skipped if they do appear in the TestExpectations file (as skips)?

If so, I actually ran into this exact same problem a couple days ago, and the patch I made (but haven't posted yet) is a change to add --skipped=expectations which means "skip the tests listed in the expectations file even if they were listed on in a text file or on the command line". It's a ~3 line change :).

Would that work for you?
Comment 3 Csaba Osztrogonác 2012-10-03 12:06:07 PDT
Ah, I can understand the intention of the implemetation too. My use case is
that I don't want to run all tests, but a small subset of tests. But it would
be great if somehow TestExpectations would honored.

Now the following 2 commands are same: (--test-list implicate --skipped=ignore)
- run-webkit-tests --test-list=./list.txt
- run-webkit-tests --test-list=./list.txt --skipped=ignore

What if we can override it with explicit --skipped=default option?
- run-webkit-tests --test-list=./list.txt --skipped=default

Additionally it would we great if we add a comment to the help of 
--test-list option to make it clear it implicates --skipped=ignore.

How does it sound?
Comment 4 Dirk Pranke 2012-10-03 12:28:28 PDT
(In reply to comment #3)
> Ah, I can understand the intention of the implemetation too. My use case is
> that I don't want to run all tests, but a small subset of tests. But it would
> be great if somehow TestExpectations would honored.
> 
> Now the following 2 commands are same: (--test-list implicate --skipped=ignore)
> - run-webkit-tests --test-list=./list.txt
> - run-webkit-tests --test-list=./list.txt --skipped=ignore
> 
> What if we can override it with explicit --skipped=default option?
> - run-webkit-tests --test-list=./list.txt --skipped=default
> 
> Additionally it would we great if we add a comment to the help of 
> --test-list option to make it clear it implicates --skipped=ignore.
> 
> How does it sound?

That's almost exactly what my patch does. I'll post it shortly.

-- Dirk
Comment 5 Dirk Pranke 2012-10-03 20:03:02 PDT
Created attachment 167026 [details]
Patch
Comment 6 Balazs Kelemen 2012-10-04 01:09:22 PDT
It's nice that you fixed it, I was also struggling with that.
Comment 7 Csaba Osztrogonác 2012-10-04 02:51:44 PDT
Comment on attachment 167026 [details]
Patch

Thanks for the quick fix.
Comment 8 WebKit Review Bot 2012-10-04 03:18:45 PDT
Comment on attachment 167026 [details]
Patch

Clearing flags on attachment: 167026

Committed r130381: <http://trac.webkit.org/changeset/130381>
Comment 9 WebKit Review Bot 2012-10-04 03:18:48 PDT
All reviewed patches have been landed.  Closing bug.