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).
Created attachment 177203 [details] Patch
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.
Adding some earlier authors/reviewers in CC in hope of a review (or just opinion on the number of "random valid values" tests).
*** Bug 87772 has been marked as a duplicate of this bug. ***
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.
(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).
Created attachment 177304 [details] patch with every test preserved
Created attachment 177312 [details] patch with every test preserved + changelog typo fixed
Comment on attachment 177312 [details] patch with every test preserved + changelog typo fixed I think this looks reasonable. r=me.
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
Created attachment 177433 [details] Reupload. Please behave, commit-queue
Created attachment 177435 [details] Reupload. Please behave, commit-queue
Comment on attachment 177435 [details] Reupload. Please behave, commit-queue Clearing flags on attachment: 177435 Committed r136486: <http://trac.webkit.org/changeset/136486>
All reviewed patches have been landed. Closing bug.