Bug 88458 – [GTK] Combine WebKit API tests into fewer binaries

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

Instead of compiling each series of tests into a separate binary, we can greatly simplify the API test compilation by having WTF tests in one binary and WebKit2 tests in another.

[Martin Robinson]

Created attachment 146156 [details]
Patch

[Martin Robinson]

Comment on attachment 146156 [details]
Patch

I noticed some issues with the new makefile. I'll fix and upload a new patch.

[Martin Robinson]

Created attachment 146394 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 146394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146394&action=review

I think the patch breaks the option --skipped=ignore

> Tools/ChangeLog:19
> +        * gtk/run-api-tests: Change the way that tests are skipped by splitting out the
> +        concept of skipping a test and skipping a suite (program) of tests. Test cases are
> +        skipped because of legitimate failures, but entire programs are skipped because of
> +        problems in the harness. As of right now a test program is only skipped if the
> +        accessibility bus cannot be started.

I think I prefer the simple approach of having only one way of handling skipped tests, the SkippedTest class already contains the program name after all. If the skipped tests doesn't contain a test case, the whole program will be skipped.

> Tools/ChangeLog:21
> +        (SkippedTest.__init__): Make the test case a required argument and have one skipped
> +        test case per SkippedTest instance.

This makes a lot of sense, since different test cases in the same suite can fail for different reasons.

> Tools/TestWebKitAPI/GNUmakefile.am:56
> +	Tools/TestWebKitAPI/gtk/main.cpp \
> +	Tools/TestWebKitAPI/Test.h \
> +	Tools/TestWebKitAPI/TestsController.cpp \
> +	Tools/TestWebKitAPI/TestsController.h \

These are common and part of libTestWebKitAPIMain

[Martin Robinson]

Comment on attachment 146394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146394&action=review

>> Tools/ChangeLog:19
>> +        accessibility bus cannot be started.
> 
> I think I prefer the simple approach of having only one way of handling skipped tests, the SkippedTest class already contains the program name after all. If the skipped tests doesn't contain a test case, the whole program will be skipped.

So perhaps the SkippedTest class could have a constructor argument that says whether or not the test is affected by the --skipped argument. I think it's still useful to skip the a11y test when the bus fails to start (regardless of the command-line arguments).

>> Tools/TestWebKitAPI/GNUmakefile.am:56
>> +	Tools/TestWebKitAPI/TestsController.h \
> 
> These are common and part of libTestWebKitAPIMain

Nice catch. I'll remove them.

[Martin Robinson]

(In reply to comment #4)
 
> I think the patch breaks the option --skipped=ignore

Oh, missed this! How does it break --skipped=ignore?

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 146394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146394&action=review
> 
> >> Tools/ChangeLog:19
> >> +        accessibility bus cannot be started.
> > 
> > I think I prefer the simple approach of having only one way of handling skipped tests, the SkippedTest class already contains the program name after all. If the skipped tests doesn't contain a test case, the whole program will be skipped.
> 
> So perhaps the SkippedTest class could have a constructor argument that says whether or not the test is affected by the --skipped argument. I think it's still useful to skip the a11y test when the bus fails to start (regardless of the command-line arguments).

You don't need a constructor argument, if the skipped test doesn't have any test case the whole program is skipped. 
Yes, I agree we should skip the program if the a11y bus fails to start regardless of the --skipped.

> >> Tools/TestWebKitAPI/GNUmakefile.am:56
> >> +	Tools/TestWebKitAPI/TestsController.h \
> > 
> > These are common and part of libTestWebKitAPIMain
> 
> Nice catch. I'll remove them.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #4)
> 
> > I think the patch breaks the option --skipped=ignore
> 
> Oh, missed this! How does it break --skipped=ignore?

Because you are not readin that option anymore, ignore means runs the tests even if they are skipped (useful to detect skipped tests that now pass), except for cases where we want to skip the whole program like the a11y bus not started.

[Martin Robinson]

Created attachment 147911 [details]
Patch

[Martin Robinson]

(In reply to comment #8)

> Because you are not readin that option anymore, ignore means runs the tests even if they are skipped (useful to detect skipped tests that now pass), except for cases where we want to skip the whole program like the a11y bus not started.

--ignore=skip was in fact working, but --ignore=only has been broken for a while. Carlos agreed that we can fix that in a followup patch.  I've addressed his other concerns in this new patch.

[Carlos Garcia Campos]

Comment on attachment 147911 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147911&action=review

Looks good to me, please fix the a11y skipped test before landing.

> Tools/gtk/run-api-tests:160
> +            self._skipped_tests.append(SkippedTest(WebKit2APITests/TestWebKitAccessibility, SkippedTest.ENTIRE_SUITE, "Could not start accessibility bus", 0))

I think this line is longer than 120, it would be easier to read if it's split, I think. What is the last 0? the bug number? You don't need to give that parameter as it takes None by default and we explicitly check for None. Does it even work? the test name is missing quotation marks

[Martin Robinson]

Committed r120944: <http://trac.webkit.org/changeset/120944>