Bug 78213 – run-webkit-tests should handle empty testlist

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Nandor Huszka]

If we give an empty testlist to the run-webkit-tests it runs all the tests.

[Nandor Huszka]

Created attachment 126258 [details]
Patch

It checks also whether testlist is exists.

[Nandor Huszka]

Created attachment 126288 [details]
Patch

Fix indentation issues.

[Csaba Osztrogonác]

(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.

[Nandor Huszka]

Created attachment 126503 [details]
Patch

(In reply to comment #3)

I have moved the modifications to the new-run-webkit-tests.

[Eric Seidel (no email)]

Comment on attachment 126503 [details]
Patch

This needs unittests.  Also free functions are generally discouraged (as they're difficult to mock).

[Dirk Pranke]

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.

[Csaba Osztrogonác]

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.

[Dirk Pranke]

(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?

[Csaba Osztrogonác]

(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.

[Dirk Pranke]

closing this as wontfix ...