Bug 78213 - run-webkit-tests should handle empty testlist
Summary: run-webkit-tests should handle empty testlist
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: NRWT
Depends on:
Blocks: 78699
  Show dependency treegraph
 
Reported: 2012-02-09 01:14 PST by Nandor Huszka
Modified: 2012-06-19 14:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.57 KB, patch)
2012-02-09 01:22 PST, Nandor Huszka
no flags Details | Formatted Diff | Diff
Patch (1.66 KB, patch)
2012-02-09 05:39 PST, Nandor Huszka
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2012-02-10 05:09 PST, Nandor Huszka
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nandor Huszka 2012-02-09 01:14:38 PST
If we give an empty testlist to the run-webkit-tests it runs all the tests.
Comment 1 Nandor Huszka 2012-02-09 01:22:26 PST
Created attachment 126258 [details]
Patch

It checks also whether testlist is exists.
Comment 2 Nandor Huszka 2012-02-09 05:39:00 PST
Created attachment 126288 [details]
Patch

Fix indentation issues.
Comment 3 Csaba Osztrogonác 2012-02-09 07:01:07 PST
(In reply to comment #2)
> Created an attachment (id=126288) [details]
> Patch

--test-list is an NRWT parameter, so NRWT should handle 
it, not the run-webkit-tests perl wrapper script.

And the return value of the NRWT should be 0 if the 
file given in --test-list has empty content.
Comment 4 Nandor Huszka 2012-02-10 05:09:06 PST
Created attachment 126503 [details]
Patch

(In reply to comment #3)

I have moved the modifications to the new-run-webkit-tests.
Comment 5 Eric Seidel (no email) 2012-02-10 10:26:42 PST
Comment on attachment 126503 [details]
Patch

This needs unittests.  Also free functions are generally discouraged (as they're difficult to mock).
Comment 6 Dirk Pranke 2012-02-10 16:11:10 PST
I could be convinced that passing an empty file to --test-list should not result in running all the tests, but why would we want it to do nothing and return success instead of returning an error? (This is an honest question; maybe you have some need for this behavior)?

That said, this logic does not belong in the new-run-webkit-tests wrapper script; it needs to be next to the other logic for handling --test-file, and the code should be using the host/filesystem abstraction rather than calling os.path directly. 

Also, I agree w/ Eric's comments re: needing tests.
Comment 7 Csaba Osztrogonác 2012-02-15 05:55:50 PST
We need this change for selective testing. (see https://bugs.webkit.org/show_bug.cgi?id=78699 for details) If a change doesn't affect any tests, run-selective-webkit-tests would call run-webkit-tests with empty testlist.
Now if you give an empty testlist to NRWT, it runs all tests. I think it shouldn't run any test and return with success (0), because a failing return code won't be good if we want to use it on a buildbot or in an EWS.
Comment 8 Dirk Pranke 2012-02-15 18:29:23 PST
(In reply to comment #7)
> We need this change for selective testing. (see https://bugs.webkit.org/show_bug.cgi?id=78699 for details) If a change doesn't affect any tests, run-selective-webkit-tests would call run-webkit-tests with empty testlist.
> Now if you give an empty testlist to NRWT, it runs all tests. I think it shouldn't run any test and return with success (0), because a failing return code won't be good if we want to use it on a buildbot or in an EWS.

Okay, I now understand the context a bit better, but if you have no tests, why invoke run-webkit-tests at all? Can't you just short-circuit things in run-selective-webkit-tests? Given that it's not clear to me that this is generally the behavior one would want, it seems like getting custom behavior is better enforced in the caller, no?
Comment 9 Csaba Osztrogonác 2012-02-15 22:34:09 PST
(In reply to comment #8)
> Okay, I now understand the context a bit better, but if you have no tests, why invoke run-webkit-tests at all? Can't you just short-circuit things in run-selective-webkit-tests? Given that it's not clear to me that this is generally the behavior one would want, it seems like getting custom behavior is better enforced in the caller, no?

I thought over, you're absolutely right. Checking it in run-selective-webkit-tests is simpler and reasonable. We will do it.

(In reply to comment #6)
> I could be convinced that passing an empty file to --test-list should not result in running all the tests, but why would we want it to do nothing and return success instead of returning an error? (This is an honest question; maybe you have some need for this behavior)?

After rethinking this one too, I support returning an 
error code if we still pass an empty testlist to NRWT.
Comment 10 Dirk Pranke 2012-06-19 14:42:41 PDT
closing this as wontfix ...