Closed
Bug 1036992
Opened 10 years ago
Closed 10 years ago
Investigate splitting test_seek.html into different tests
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: cmtalbert, Assigned: sydpolk)
References
Details
Attachments
(1 file, 5 obsolete files)
47.58 KB,
patch
|
cmtalbert
:
review+
|
Details | Diff | 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 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
And this version removes Clint's debugging code....
Attachment #8462861 -
Attachment is obsolete: true
Reporter | ||
Comment 14•10 years ago
|
||
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)
Reporter | ||
Comment 15•10 years ago
|
||
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)
Reporter | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8462861 -
Flags: review?(cpearce)
Assignee | ||
Comment 18•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(ctalbert)
Reporter | ||
Comment 19•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/174637171dbe
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•