Closed Bug 1036992 Opened 10 years ago Closed 10 years ago

Investigate splitting test_seek.html into different tests

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: cmtalbert, Assigned: sydpolk)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch The patch for try (obsolete) — Splinter Review
As a result of https://bugzilla.mozilla.org/show_bug.cgi?id=981153#c904 in bug 981153, Anthony asked me whether we could look into splitting up test_seek.html into component tests. It's still kind of open as to whether or not that would have much effect. But early testing on try shows that it might.

I'll attach my patch, and note that in my patch I reverted the change that did not run the test on Android and B2G, so that I could more clearly and easily see the effect of the intermittent failures in the original test compared to the split-out tests.

The splitting was done in a really brain-dead simple fashion mostly because I didn't want to invalidate the kinds of testing that the test is already doing. I just wanted to mitigate the fact that it was doing too much in one test.

Here is the try push: https://tbpl.mozilla.org/?tree=Try&rev=49c86056b066

If this is the way we want to go, then we'll need to take test_seek.html out of the rotation and just use the split-out tests, so there will need to be at least one more revision on the patch. I'll invite Cpearce to have a look.
Attachment #8453830 - Flags: feedback?(cpearce)
Comment on attachment 8453830 [details] [diff] [review]
The patch for try

Review of attachment 8453830 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this is roughly what I had in mind.

::: content/media/test/test_seek-split1.html
@@ +21,5 @@
> +</head>
> +<body>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +

Could everything inside this <script> tag also be put in an external js file that is loaded in every test_seek-split file, to reduce duplication?
Attachment #8453830 - Flags: feedback?(cpearce) → feedback+
Flags: needinfo?(ctalbert)
(In reply to Syd Polk from comment #2)
> Created attachment 8459786 [details] [diff] [review]
> Combine previous patch; set PARALLEL_TASKS to 1 for test2 and test11.

Syd, try push: https://tbpl.mozilla.org/?tree=Try&rev=fc69e234fb84
Flags: needinfo?(ctalbert)
I have looked at the results from https://tbpl.mozilla.org/?tree=Try&rev=fc69e234fb84, and they seen to have helped. Whereas one failure from the uber-huge test_seek.html still happens, nothing of the other test_seek-split* failed, where as last patch test_seek_split2 and test_seek_split11 failed.

This seems to be progress.

My guess is that the next step is to do the refactoring that cpearce mentioned.
Flags: needinfo?(ctalbert)
Attached patch 1036992.draft3.patch (obsolete) — Splinter Review
Here is the latest patch. It does the following:

- Refactors common javascript in test_seek_split*.html into a file named seek_support.js.
- Adds that file to mochitest.ini.
- Keeps PARALLEL_TESTS at 1 for seek2 and seek11.
- Removes test_seek.html.

Anything else need to be done?
Attachment #8453830 - Attachment is obsolete: true
Attachment #8459786 - Attachment is obsolete: true
Flags: needinfo?(ctalbert)
I prefer the name |test_seek-1.html| over |test_seek-split1.html| since the file name should convey the purpose of the test instead of how it is structured internally.

Btw, we should also move the code in seek1.js to test_seek-split1.html and remove seek1.js for now it is less meaningful for seek*.js to stand alone.

Overall it looks fine to me.
Comment on attachment 8461145 [details] [diff] [review]
1036992.draft3.patch

Review of attachment 8461145 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks good to me. I'm not sure if we still need the request longer timeout call in seek_split now that the test has been split out. But in order to test that I will upload to try.

And one nit, make sure to not add spaces at ends of lines or on blank lines (there are sometimes editor settings to help with this). That's just a mozilla style thing.

::: content/media/test/seek_support.js
@@ +31,5 @@
> +}
> +
> +function startTest(test, token) {
> +  var v = document.createElement('video');
> +  dump("SEEK-TEST-DBG_Clint: test.name: " + test.name + " test.number: " + test.number + "\n");

We should remove this ^ debugging line before we land it.
(In reply to JW Wang [:jwwang] from comment #7)
> I prefer the name |test_seek-1.html| over |test_seek-split1.html| since the
> file name should convey the purpose of the test instead of how it is
> structured internally.
> 
> Btw, we should also move the code in seek1.js to test_seek-split1.html and
> remove seek1.js for now it is less meaningful for seek*.js to stand alone.
> 
> Overall it looks fine to me.

I like both of these suggestions since the seek*.js files aren't used anywhere else it makes sense to put them in the split out html files now.

Syd, would you please work up another patch with these changes (plus removing that debug line) and we can put that on try?
Flags: needinfo?(spolk)
Will do.
Flags: needinfo?(spolk)
Attached patch 1036992-patch4.diff (obsolete) — Splinter Review
Incorporated all feedback so far. I think this is ready. Clint, could you run this on try?
Attachment #8461145 - Attachment is obsolete: true
Flags: needinfo?(ctalbert)
Added latest version of patch.
Flags: needinfo?(ctalbert)
Attached patch 1036992-patch5.diff (obsolete) — Splinter Review
And this version removes Clint's debugging code....
Attachment #8462861 - Attachment is obsolete: true
Comment on attachment 8462861 [details] [diff] [review]
1036992-patch4.diff

Review of attachment 8462861 [details] [diff] [review]:
-----------------------------------------------------------------

Only one nit as I did a quick review. This is live on try now:  https://tbpl.mozilla.org/?tree=Try&rev=6b039584ba0c

Let's retrigger a bunch and see if we can convince ourselves that we are ready to land.

Thanks a bunch for the patch, Syd.  I'm going to flag cpearce for the official review.

Cpearce - I'd also like your opinion on renaming test_seek2.html to test_seek_invalid.html (since it tests invalid seek operations) so that it isn't mistaken with test_seek-2.html (the split out seek test from the original test_seek.html). 

For best results, I enabled test_seek2.html before pushing to try so that we get representative samples up there. 

r+ from me, pending the notes below and the try results.

::: content/media/test/mochitest.ini
@@ -419,5 @@
>  skip-if = true # bug 1021673
>  [test_seek_out_of_range.html]
> -[test_seek.html]
> -skip-if = buildapp == 'b2g' || toolkit == 'android' # Intermittent test failures in bug 1023564 and bug 981153
> -[test_seek2.html]

You accidentally removed test_seek2.html which is a valid seek test that we should keep around and isn't at all related to the split-out seek test. Because you are absolutely not going to be the last person to make this mistake I think we should rename test_seek2.html to something else. Looking at the content of the test, I think we should all it test_seek_invalid.html.

::: content/media/test/seek_support.js
@@ +31,5 @@
> +}
> +
> +function startTest(test, token) {
> +  var v = document.createElement('video');
> +  dump("SEEK-TEST-DBG_Clint: test.name: " + test.name + " test.number: " + test.number + "\n");

We still need to remember to take this debug line out of the final patch before we push.
Attachment #8462861 - Flags: review?(cpearce)
Comment on attachment 8462872 [details] [diff] [review]
1036992-patch5.diff

Review of attachment 8462872 [details] [diff] [review]:
-----------------------------------------------------------------

Syd, r+, you and I hit bugzilla at nearly the same time.

Cpearce, flagging you for official review on the latest version of the patch and on the question in my above comment around the naming of test_seek2.html
Attachment #8462872 - Flags: review?(cpearce)
Sheriffs: FYI
This is an outgrowth of the orange on bug 981153 (intermittent failures on test_seek.html). This patch splits up test_seek.html into different tests allowing us finer granularity on which parts of the test are actually intermittent, and has proven (in earlier try runs) to be less intermittent overall than the composite test.

The likely reason for this is that as one massive test, test_seek.html opened hundreds of video elements in one page and then used asynchronous event handlers to manage those elements some of which would then fire in all kinds of wacky orders. Reducing the complexity of the test seems to have been a win here, but of course we won't really know until we hit inbound, so you're being cc'd at this point.

As part of this work, we are probably going to rename test_seek2.html which is not a tracked intermittent so that should be fine, but I wanted to let you know in either case.
Comment on attachment 8462872 [details] [diff] [review]
1036992-patch5.diff

Review of attachment 8462872 [details] [diff] [review]:
-----------------------------------------------------------------

renaming the old test_seek_2 to test_seek_invalid is fine.

Thanks for doing this.
Attachment #8462872 - Flags: review?(cpearce) → review+
Attached patch 1036992.patch6Splinter Review
Renamed test_seek2.html to test_invalid_seek.html. Debugging code is gone. Subsumes all changes so far.
Attachment #8462872 - Attachment is obsolete: true
Flags: needinfo?(ctalbert)
Flags: needinfo?(ctalbert)
Comment on attachment 8465769 [details] [diff] [review]
1036992.patch6

Adding back cpearce's review that Syd didn't realize he could carry forward.
Attachment #8465769 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/174637171dbe
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1056629
You need to log in before you can comment on or make changes to this bug.