Bug 103744 – Make svg/dom/viewspec-parser-*.html non-flaky

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jussi Kukkonen (jku)]

Filing this so I don't forget: svg/dom/viewspec-parser-*.html are flaky.

Bug 87726 tried to fix the situation but AFAICT based on the results.txt it only created 5 tests that all individually take as long as the original one test did...

Should either figure out how to test this area without this semi-random fuzzing or implement what bug 87726 tried to implement (divide the tests into smaller sets).

[Jussi Kukkonen (jku)]

Created attachment 177203 [details]
Patch

[Jussi Kukkonen (jku)]

Comment on attachment 177203 [details]
Patch

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

> LayoutTests/ChangeLog:14
> +        Modified viewspec-parser-*.html in three ways:
> +        - remove duplicate testing: the five files were all running
> +          the exact same tests (only a few percent were unique).
> +        - move the test division out of viewspec-parser.js to make it more
> +          visible, divide the tests into logical sets.
> +        - cut down on the number of 'random valid values' tests: Instead of
> +          300 tests, run 72.

The first two points should be no-brainers and I've compared the output
to the current output: all the tests are included and the results are same, just faster.

The last item is just my suggestion: I can add a couple more test files to run the rest
of the 'random valid values' tests if that's wanted. I'm not sure that adds much real
coverage though, adding some intelligence to the fuzzer might be a better option. E.g.
using the known argument numbers/types for the attributes to produce more interesting
test material.

[Jussi Kukkonen (jku)]

Adding some earlier authors/reviewers in CC in hope of a review (or just opinion on the  number of "random valid values" tests).

[Dirk Schulze]

*** Bug 87772 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Comment on attachment 177203 [details]
Patch

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

You removed by far to  many test scenarios. Something that might work for me is splitting the test into multiple files and keep the flacky parts in the original parser files. Exponent parsing and correct comma parsing is very necessary.

> LayoutTests/svg/dom/viewspec-parser-1-expected.txt:-73
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(74- 9.(+ e.0e635
> -Parsed: [initial view] from: svgView(viewBox(74- 9.(+ e.0e635
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(-9,).8e7e4e9ee
> -Parsed: [initial view] from: svgView(viewBox(-9,).8e7e4e9ee 
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(49-,,4)30378.052
> -Parsed: [initial view] from: svgView(viewBox(49-,,4)30378.052
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(e7863e
> -Parsed: [initial view] from: svgView(viewBox(e7863e
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(0
> -Parsed: [initial view] from: svgView(viewBox(	0
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(e72(e,49.7.
> -Parsed: [initial view] from: svgView(viewBox(e72(	e	,49.7.
> -
> -Loaded: resources/viewspec-target.svg#svgView(viewBox(e55,
> -Parsed: [initial view] from: svgView(viewBox(e55, 

All this seems not to be covered by the test anymore.

[Jussi Kukkonen (jku)]

(In reply to comment #5)
> (From update of attachment 177203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177203&action=review
> 
> You removed by far to  many test scenarios. Something that might work for me is splitting the test into multiple files and keep the flacky parts in the original parser files. Exponent parsing and correct comma parsing is very necessary.

There's no flaky parts as such as far as I can tell -- the tests would just timeout  because they contained hundreds of parsing tests and every test modifies the dom a couple of times (including adding an iframe)...

> > LayoutTests/svg/dom/viewspec-parser-1-expected.txt:-73
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(74- 9.(+ e.0e635
> > -Parsed: [initial view] from: svgView(viewBox(74- 9.(+ e.0e635
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(-9,).8e7e4e9ee
> > -Parsed: [initial view] from: svgView(viewBox(-9,).8e7e4e9ee 
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(49-,,4)30378.052
> > -Parsed: [initial view] from: svgView(viewBox(49-,,4)30378.052
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(e7863e
> > -Parsed: [initial view] from: svgView(viewBox(e7863e
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(0
> > -Parsed: [initial view] from: svgView(viewBox(	0
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(e72(e,49.7.
> > -Parsed: [initial view] from: svgView(viewBox(e72(	e	,49.7.
> > -
> > -Loaded: resources/viewspec-target.svg#svgView(viewBox(e55,
> > -Parsed: [initial view] from: svgView(viewBox(e55, 
> 
> All this seems not to be covered by the test anymore.

I've checked these, and every example listed here is actually tested by viewspec-parser-3.html in my patch. Anyway, as I said, I'll be happy to include all the old tests if that seems safer.

I don't know how the parser actually works but looking at the tests I'm guessing the "Random assortments of valid characters" tests (viewspec-parser-3.html in my patch) only test the part of the parser where it decides if the attribute signature is valid or not... and almost none of them are actually valid. My guess is the arguments are never parsed or at least the tester can't tell if they are (as we only get "[initial view]" as the parsed value).

[Jussi Kukkonen (jku)]

Created attachment 177304 [details]
patch with every test preserved

[Jussi Kukkonen (jku)]

Created attachment 177312 [details]
patch with every test preserved + changelog typo fixed

[Dirk Schulze]

Comment on attachment 177312 [details]
patch with every test preserved + changelog typo fixed

I think this looks reasonable. r=me.

[WebKit Review Bot]

Comment on attachment 177312 [details]
patch with every test preserved + changelog typo fixed

Rejecting attachment 177312 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
it line 152.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
From http://git.chromium.org/external/Webkit
 + 54297d7...12af24d HEAD       -> origin/HEAD  (forced update)
error: Ref refs/remotes/origin/master is at 12af24d2f26725d342ea591d58b957f0268c6ca8 but expected 54297d74214132a39292f05bb9466fdab588b0aa
 ! 54297d7...12af24d master     -> origin/master  (unable to update local ref)
Died at Tools/Scripts/update-webkit line 152.

Full output: http://queues.webkit.org/results/15121639

[Jussi Kukkonen (jku)]

Created attachment 177433 [details]
Reupload. Please behave, commit-queue

[Jussi Kukkonen (jku)]

Created attachment 177435 [details]
Reupload. Please behave, commit-queue

[WebKit Review Bot]

Comment on attachment 177435 [details]
Reupload. Please behave, commit-queue

Clearing flags on attachment: 177435

Committed r136486: <http://trac.webkit.org/changeset/136486>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.