Bug 92627 - [Qt][NRWT] REGRESSION(123729): Forcing pixel tests with -p doesn't work
Summary: [Qt][NRWT] REGRESSION(123729): Forcing pixel tests with -p doesn't work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: NRWT, Qt, QtTriaged
Depends on:
Blocks: 91754
  Show dependency treegraph
 
Reported: 2012-07-30 04:12 PDT by Csaba Osztrogonác
Modified: 2012-07-30 12:21 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.61 KB, patch)
2012-07-30 04:43 PDT, Balazs Kelemen
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-07-30 04:12:21 PDT
"run-webkit-tests --pixel-tests --pixel-test-directory X" works fine, but 
"run-webkit-tests --pixel-tests X.html Y.html Z-directory" doesn't run pixel tests.

I think the new --pixel-test-directory shouldn't affect the original 
behaviour of NRWT when we don't use this option.

Could you check it, please?
Comment 1 Balazs Kelemen 2012-07-30 04:37:09 PDT
This is because I made compositing the default directory for pixel testing. Maybe it was not the best idea. My idea was that maybe we want to run pixel test by default but only for the test cases that affects accelerated compositing. I made it to be a default so that developers run the same set of pixel tests as bots (without the need to know some special configuration).
One workaround could be to accept globs, so one could say --pixel-test-directory *. However, given that it's still not very clear how we want to go on with pixel testing, I could rather just remove this default behavior, so that --pixel-tests will turn on pixel testing for all tests again.
Comment 2 Balazs Kelemen 2012-07-30 04:43:26 PDT
Created attachment 155257 [details]
Patch
Comment 3 Csaba Osztrogonác 2012-07-30 04:45:20 PDT
(In reply to comment #1)
> This is because I made compositing the default directory for pixel testing. Maybe it was not the best idea. My idea was that maybe we want to run pixel test by default but only for the test cases that affects accelerated compositing. I made it to be a default so that developers run the same set of pixel tests as bots (without the need to know some special configuration).
> One workaround could be to accept globs, so one could say --pixel-test-directory *. However, given that it's still not very clear how we want to go on with pixel testing, I could rather just remove this default behavior, so that --pixel-tests will turn on pixel testing for all tests again.

Ah, I see it now. The idea was good to switch on pixel tests by default for
compositing dir. But unfortunately its sideeffect was that we can't run pixel
tests for other dirs without passing --pixel-test-directory ...

Now I don't have idea which workaround would be better ... Don't hurry, 
we have time to find the best solution. ;-) I'm examining the status,
stability, ... of pixel testing after the big rebaseline.
Comment 4 Balazs Kelemen 2012-07-30 04:48:34 PDT
> Ah, I see it now. The idea was good to switch on pixel tests by default for
> compositing dir. 

No, I was not enabled pixel testing by default, but restricted --pixel-tests to compositing (by default).
Comment 5 Csaba Osztrogonác 2012-07-30 06:21:43 PDT
Comment on attachment 155257 [details]
Patch

Clearing flags on attachment: 155257

Committed r124020: <http://trac.webkit.org/changeset/124020>
Comment 6 Csaba Osztrogonác 2012-07-30 06:21:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Dirk Pranke 2012-07-30 12:21:01 PDT
It seems like there's four different things we might want to do ...

1) don't run any pixel tests

2) run a default set

3) run a custom set

4) run all of them

One option would be to introduce a magic directory name (e.g., "default"), and you could then say "--pixel-test-directory default --pixel-test-directory svg" , etc. Another option (that I like less) would be to add another command line flag. Another option would be to not have the default set, but it seems like if this "only run some of them" idea is a good permanent solution we'll really want a default set.