Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766473002:
[Media Router] Further split media_router_container tests. (Closed)
Description[Media Router] Further split media_router_container tests.
This change splits the media_router_container_tests.js file into two separate
test files, based on test timing, to help with the timeout issue these tests
have been experiencing on bots.
BUG=591227
R=imcheng@chromium.org
Committed: https://crrev.com/e144192a9b4776bc10e7c4c2dc0e369ceac763c6
Cr-Commit-Position: refs/heads/master@{#380687}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Responding to comments #
Total comments: 4
Patch Set 3 : Logical test file split #
Total comments: 2
Patch Set 4 : Collected common utility functions #
Total comments: 12
Patch Set 5 : Alphabetize, comments, and nits #
Total comments: 15
Patch Set 6 : Fixed doc comments #Patch Set 7 : Fixed function param docs #Messages
Total messages: 24 (6 generated)
Files were split based on test timing to make them roughly even. PTAL.
imcheng@chromium.org changed reviewers: + apacible@chromium.org
lgtm added Jennifer to take a look as well. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_container_part1_tests.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. If there is there a tracking bug for this, please link it here. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', I see you have moved most of the cast mode list related tests into one file. Would it make sense for call this media_router_container_cast_mode_list.js, or something similar? https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:95: TEST_F('MediaRouterElementsBrowserTest', Since we are at the point where it is necessary to split tests within a logical component to prevent timing out, it may be worthwhile to put in a comment here explaining that. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:101: // Run all registered tests. This comment can be removed from all test cases.
https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_container_part1_tests.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:5: /** @fileoverview Suite of tests for media-router-container. */ How did you decide how to split up tests? Is this is the most tests we can have here before it starts timing out again? When someone adds a new test, where should they add it to avoid timeout issues? https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:6: cr.define('media_router_container_part1', function() { nit: spell out the numbers. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. On 2016/03/04 00:26:58, imcheng wrote: > If there is there a tracking bug for this, please link it here. In what case does the browser window become unfocused? https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:98: // Register mocha tests for the container. This comment can also be removed from all test cases.
I have responded to all the comments. PTAL. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_container_part1_tests.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:5: /** @fileoverview Suite of tests for media-router-container. */ On 2016/03/04 01:25:28, apacible wrote: > How did you decide how to split up tests? Is this is the most tests we can have > here before it starts timing out again? When someone adds a new test, where > should they add it to avoid timeout issues? Unfortunately I can't reproduce the timeouts. So the hope is to divide the files roughly evenly by execution time. I think this is probably as large as the files should get, and they might still need to be smaller. As a point of reference, these are still larger than most of the settings page's tests and before the split this test was about as large as one of their flaky tests that has been marked to be split. Do you think this is reasonable or should we try to define logical splits instead? I agree that coming upon this later could be confusing. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:6: cr.define('media_router_container_part1', function() { On 2016/03/04 01:25:28, apacible wrote: > nit: spell out the numbers. Done. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. On 2016/03/04 01:25:28, apacible wrote: > On 2016/03/04 00:26:58, imcheng wrote: > > If there is there a tracking bug for this, please link it here. > > In what case does the browser window become unfocused? If I manually switch back to the console to watch output or do something else. This isn't an issue on bots but I thought I'd leave a comment in case it comes up again later. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. On 2016/03/04 00:26:58, imcheng wrote: > If there is there a tracking bug for this, please link it here. Done. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', On 2016/03/04 00:26:58, imcheng wrote: > I see you have moved most of the cast mode list related tests into one file. > Would it make sense for call this media_router_container_cast_mode_list.js, or > something similar? Well, this isn't really a logical split. It was based on evenly splitting the execution time. If more cast mode tests were going to be added, they might need to go in a new file. So I don't think I want to suggest that file as a place that cast mode tests go. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:95: TEST_F('MediaRouterElementsBrowserTest', On 2016/03/04 00:26:59, imcheng wrote: > Since we are at the point where it is necessary to split tests within a logical > component to prevent timing out, it may be worthwhile to put in a comment here > explaining that. Done. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:98: // Register mocha tests for the container. On 2016/03/04 01:25:29, apacible wrote: > This comment can also be removed from all test cases. Done. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:101: // Run all registered tests. On 2016/03/04 00:26:58, imcheng wrote: > This comment can be removed from all test cases. Done.
https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_container_part1_tests.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:5: /** @fileoverview Suite of tests for media-router-container. */ On 2016/03/04 02:14:37, btolsch wrote: > On 2016/03/04 01:25:28, apacible wrote: > > How did you decide how to split up tests? Is this is the most tests we can > have > > here before it starts timing out again? When someone adds a new test, where > > should they add it to avoid timeout issues? > > Unfortunately I can't reproduce the timeouts. So the hope is to divide the files > roughly evenly by execution time. I think this is probably as large as the files > should get, and they might still need to be smaller. As a point of reference, > these are still larger than most of the settings page's tests and before the > split this test was about as large as one of their flaky tests that has been > marked to be split. > > Do you think this is reasonable or should we try to define logical splits > instead? I agree that coming upon this later could be confusing. How long does it take to currently execute for each part? How long does the settings tests take? I'm guessing using setTimeouts() frequently contribute to the issue as well. We can use the time as a guideline for future tests to avoid the issue. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. On 2016/03/04 02:14:37, btolsch wrote: > On 2016/03/04 01:25:28, apacible wrote: > > On 2016/03/04 00:26:58, imcheng wrote: > > > If there is there a tracking bug for this, please link it here. > > > > In what case does the browser window become unfocused? > > If I manually switch back to the console to watch output or do something else. > This isn't an issue on bots but I thought I'd leave a comment in case it comes > up again later. Ah. This is expected behavior. :) Browser tests are meant to run independently and in isolation. In general, try to run browser tests locally without switching windows or clicking around. I typically switch to working on another machine when I'm waiting for browser tests (or other tests with a UI) to run. You can remove this comment. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', On 2016/03/04 02:14:38, btolsch wrote: > On 2016/03/04 00:26:58, imcheng wrote: > > I see you have moved most of the cast mode list related tests into one file. > > Would it make sense for call this media_router_container_cast_mode_list.js, or > > something similar? > > Well, this isn't really a logical split. It was based on evenly splitting the > execution time. If more cast mode tests were going to be added, they might need > to go in a new file. So I don't think I want to suggest that file as a place > that cast mode tests go. Would it make sense to split tests based on views or specific things they're testing? That'll hopefully bring the execution time down by a larger margin for each test while making it more organized. For example, media_router_container_first_run_flow.js, media_router_container_cast_mode_list.js, etc... Right now, anyone looking for tests will have to look at both files. https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:88: // Register mocha tests for the issue banner. Also remove comments here. https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:90: nit: The newline here can also be removed from all test cases.
After testing the timing, I'm both not sure that it's definitely only a timing issue and not sure that a single split would fix a timing issue. The tests are further split into logical units now. This doesn't even split the time, but each test runs faster than either part before. If it's not a timeout issue, the logical split should also help narrow down the problematic test in the future. PTAL. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_container_part1_tests.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:5: /** @fileoverview Suite of tests for media-router-container. */ On 2016/03/04 17:21:33, apacible wrote: > On 2016/03/04 02:14:37, btolsch wrote: > > On 2016/03/04 01:25:28, apacible wrote: > > > How did you decide how to split up tests? Is this is the most tests we can > > have > > > here before it starts timing out again? When someone adds a new test, where > > > should they add it to avoid timeout issues? > > > > Unfortunately I can't reproduce the timeouts. So the hope is to divide the > files > > roughly evenly by execution time. I think this is probably as large as the > files > > should get, and they might still need to be smaller. As a point of reference, > > these are still larger than most of the settings page's tests and before the > > split this test was about as large as one of their flaky tests that has been > > marked to be split. > > > > Do you think this is reasonable or should we try to define logical splits > > instead? I agree that coming upon this later could be confusing. > > How long does it take to currently execute for each part? How long does the > settings tests take? I'm guessing using setTimeouts() frequently contribute to > the issue as well. > > We can use the time as a guideline for future tests to avoid the issue. It takes about 2-2.2 seconds to run each part on my desktop. The flaky settings test 'CrSettingsBrowserTest.CrSettingsTest takes the same amount of time. That being said, the filter tests take 2.2-2.4 seconds and haven't been reported as flaky. So perhaps splitting into smaller logical units would be better in addition to the clarity concern from your other comment. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_container_part1_tests.js:218: // test opens is unfocused, causing this test to fail. On 2016/03/04 17:21:33, apacible wrote: > On 2016/03/04 02:14:37, btolsch wrote: > > On 2016/03/04 01:25:28, apacible wrote: > > > On 2016/03/04 00:26:58, imcheng wrote: > > > > If there is there a tracking bug for this, please link it here. > > > > > > In what case does the browser window become unfocused? > > > > If I manually switch back to the console to watch output or do something else. > > This isn't an issue on bots but I thought I'd leave a comment in case it comes > > up again later. > > Ah. This is expected behavior. :) > > Browser tests are meant to run independently and in isolation. In general, try > to run browser tests locally without switching windows or clicking around. I > typically switch to working on another machine when I'm waiting for browser > tests (or other tests with a UI) to run. > > You can remove this comment. Done. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', On 2016/03/04 17:21:33, apacible wrote: > On 2016/03/04 02:14:38, btolsch wrote: > > On 2016/03/04 00:26:58, imcheng wrote: > > > I see you have moved most of the cast mode list related tests into one file. > > > Would it make sense for call this media_router_container_cast_mode_list.js, > or > > > something similar? > > > > Well, this isn't really a logical split. It was based on evenly splitting the > > execution time. If more cast mode tests were going to be added, they might > need > > to go in a new file. So I don't think I want to suggest that file as a place > > that cast mode tests go. > > Would it make sense to split tests based on views or specific things they're > testing? That'll hopefully bring the execution time down by a larger margin for > each test while making it more organized. > > For example, media_router_container_first_run_flow.js, > media_router_container_cast_mode_list.js, etc... > > Right now, anyone looking for tests will have to look at both files. Initially I was hesitant to do that because of the duplication in the suite setup that would cause. I think that would be worth it though, especially since the timings are implying that this split might still be flaky on bots. And if it turns out it isn't a timing issue, the logical split would help narrow down the issue. https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:88: // Register mocha tests for the issue banner. On 2016/03/04 17:21:33, apacible wrote: > Also remove comments here. Done. https://codereview.chromium.org/1766473002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:90: On 2016/03/04 17:21:33, apacible wrote: > nit: The newline here can also be removed from all test cases. Done.
https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', On 2016/03/04 20:00:37, btolsch wrote: > On 2016/03/04 17:21:33, apacible wrote: > > On 2016/03/04 02:14:38, btolsch wrote: > > > On 2016/03/04 00:26:58, imcheng wrote: > > > > I see you have moved most of the cast mode list related tests into one > file. > > > > Would it make sense for call this > media_router_container_cast_mode_list.js, > > or > > > > something similar? > > > > > > Well, this isn't really a logical split. It was based on evenly splitting > the > > > execution time. If more cast mode tests were going to be added, they might > > need > > > to go in a new file. So I don't think I want to suggest that file as a place > > > that cast mode tests go. > > > > Would it make sense to split tests based on views or specific things they're > > testing? That'll hopefully bring the execution time down by a larger margin > for > > each test while making it more organized. > > > > For example, media_router_container_first_run_flow.js, > > media_router_container_cast_mode_list.js, etc... > > > > Right now, anyone looking for tests will have to look at both files. > > Initially I was hesitant to do that because of the duplication in the suite > setup that would cause. I think that would be worth it though, especially since > the timings are implying that this split might still be flaky on bots. And if it > turns out it isn't a timing issue, the logical split would help narrow down the > issue. Let's pull out all of the properties / setup / custom checks into another JS file so it's all in one place. I agree with the duplication concern; it would be tedious to update every single file (e.g. |hiddenCheckElementIdList|) for some changes. We could probably do something similar to how settings pulls in additional files. Example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... https://codereview.chromium.org/1766473002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:97: 'MediaRouterElementsTestMediaRouterContainerCastModeList', nit: Since all the tests are under "MediaRouterElementsBrowserTest", you can remove "MediaRouterElementsTest" from all of the below. e.g. TEST_F('MediaRouterElementsBrowserTest', 'MediaRouterElementsTestMediaRouterContainerCastModeList') simplifies to... TEST_F('MediaRouterElementsBrowserTest', 'MediaRouterContainerCastModeList')
I pulled the common code for the container tests into a separate file. PTAL. https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/1/chrome/test/data/webui/medi... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:43: 'media_router_container_part2_tests.js', On 2016/03/07 17:59:04, apacible wrote: > On 2016/03/04 20:00:37, btolsch wrote: > > On 2016/03/04 17:21:33, apacible wrote: > > > On 2016/03/04 02:14:38, btolsch wrote: > > > > On 2016/03/04 00:26:58, imcheng wrote: > > > > > I see you have moved most of the cast mode list related tests into one > > file. > > > > > Would it make sense for call this > > media_router_container_cast_mode_list.js, > > > or > > > > > something similar? > > > > > > > > Well, this isn't really a logical split. It was based on evenly splitting > > the > > > > execution time. If more cast mode tests were going to be added, they might > > > need > > > > to go in a new file. So I don't think I want to suggest that file as a > place > > > > that cast mode tests go. > > > > > > Would it make sense to split tests based on views or specific things they're > > > testing? That'll hopefully bring the execution time down by a larger margin > > for > > > each test while making it more organized. > > > > > > For example, media_router_container_first_run_flow.js, > > > media_router_container_cast_mode_list.js, etc... > > > > > > Right now, anyone looking for tests will have to look at both files. > > > > Initially I was hesitant to do that because of the duplication in the suite > > setup that would cause. I think that would be worth it though, especially > since > > the timings are implying that this split might still be flaky on bots. And if > it > > turns out it isn't a timing issue, the logical split would help narrow down > the > > issue. > > Let's pull out all of the properties / setup / custom checks into another JS > file so it's all in one place. I agree with the duplication concern; it would be > tedious to update every single file (e.g. |hiddenCheckElementIdList|) for some > changes. > > We could probably do something similar to how settings pulls in additional > files. Example: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... Done. https://codereview.chromium.org/1766473002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_elements_browsertest.js (right): https://codereview.chromium.org/1766473002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_elements_browsertest.js:97: 'MediaRouterElementsTestMediaRouterContainerCastModeList', On 2016/03/07 17:59:05, apacible wrote: > nit: Since all the tests are under "MediaRouterElementsBrowserTest", you can > remove "MediaRouterElementsTest" from all of the below. > > e.g. > TEST_F('MediaRouterElementsBrowserTest', > 'MediaRouterElementsTestMediaRouterContainerCastModeList') > > simplifies to... > > TEST_F('MediaRouterElementsBrowserTest', 'MediaRouterContainerCastModeList') Done.
https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:22: // Initialize a media-router-container before each test. Here and elsewhere: move this comment to right before creating the document.createElement('...') line. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:31: fakeBlockingIssue = test_base.fakeBlockingIssue; Here and elsewhere: declare variables. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_test_base.js (right): https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:14: var fakeBlockingIssue; You can just initialize the variables here instead of doing that later in the file. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:48: * @const {!Array<string>} nit: {!Array<!string>} https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:168: //container.castModeList = fakeCastModeList; Remove? https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:174: return { nit: alphabetize and remove extra newline.
I changed the initialization in the tests but I'm not 100% sure if I commented the functions correctly as forward declarations. PTAL. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:22: // Initialize a media-router-container before each test. On 2016/03/09 17:33:15, apacible wrote: > Here and elsewhere: move this comment to right before creating the > document.createElement('...') line. Done. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:31: fakeBlockingIssue = test_base.fakeBlockingIssue; On 2016/03/09 17:33:15, apacible wrote: > Here and elsewhere: declare variables. Done. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_test_base.js (right): https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:14: var fakeBlockingIssue; On 2016/03/09 17:33:15, apacible wrote: > You can just initialize the variables here instead of doing that later in the > file. Done. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:48: * @const {!Array<string>} On 2016/03/09 17:33:15, apacible wrote: > nit: {!Array<!string>} Done. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:168: //container.castModeList = fakeCastModeList; On 2016/03/09 17:33:15, apacible wrote: > Remove? Done. https://codereview.chromium.org/1766473002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:174: return { On 2016/03/09 17:33:15, apacible wrote: > nit: alphabetize and remove extra newline. Done.
https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:6: * cast mode list. */ nit: Here and other files: move "*/" to next line. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:13: * @param {media_router.MediaRouterView} view Expected view type. ?media_router.MediaRouterView For here and other files, if the property isn't initialized here until setUp(), add a ? to denote it can be null. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_test_base.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:11: var checkCurrentView = function(view) { Please add JSDocs for functions as well. It should just match whatever you had in the other tests. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:91: fakeRouteListWithLocalRoutesOnly = [ var https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:96: var castModeBitset = 0x2 | 0x4 | 0x8; Add comments? https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:133: * @type {string} !string Here and elsewhere: if initialized where it's declared and it's not expected to be null, add a ! to denote it will never be null. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_search_highlighter_tests.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_search_highlighter_tests.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. Not sure if rietveld just isn't showing it, but were there any changes in this file? It could have been the newline at the end being deleted. It says "0 chunks", "+-1 lines, --1 lines" in the stats.
Took another stab at the comments. PTAL. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:6: * cast mode list. */ On 2016/03/10 18:11:06, apacible wrote: > nit: Here and other files: move "*/" to next line. Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:13: * @param {media_router.MediaRouterView} view Expected view type. On 2016/03/10 18:11:06, apacible wrote: > ?media_router.MediaRouterView > > For here and other files, if the property isn't initialized here until setUp(), > add a ? to denote it can be null. Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_test_base.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:11: var checkCurrentView = function(view) { On 2016/03/10 18:11:06, apacible wrote: > Please add JSDocs for functions as well. It should just match whatever you had > in the other tests. Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:91: fakeRouteListWithLocalRoutesOnly = [ On 2016/03/10 18:11:06, apacible wrote: > var Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:96: var castModeBitset = 0x2 | 0x4 | 0x8; On 2016/03/10 18:11:06, apacible wrote: > Add comments? Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_test_base.js:133: * @type {string} On 2016/03/10 18:11:06, apacible wrote: > !string > > Here and elsewhere: if initialized where it's declared and it's not expected to > be null, add a ! to denote it will never be null. Done. https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_search_highlighter_tests.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_search_highlighter_tests.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/03/10 18:11:06, apacible wrote: > Not sure if rietveld just isn't showing it, but were there any changes in this > file? It could have been the newline at the end being deleted. > > It says "0 chunks", "+-1 lines, --1 lines" in the stats. No there were no changes, I don't know why it says that.
lgtm, thanks for doing this! https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js (right): https://codereview.chromium.org/1766473002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/media_router/media_router_container_cast_mode_list_tests.js:13: * @param {media_router.MediaRouterView} view Expected view type. On 2016/03/10 19:08:08, btolsch wrote: > On 2016/03/10 18:11:06, apacible wrote: > > ?media_router.MediaRouterView > > > > For here and other files, if the property isn't initialized here until > setUp(), > > add a ? to denote it can be null. > > Done. Oops, this is a function. Function params for our tests are expected to be non-null, I believe.
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from imcheng@chromium.org, apacible@chromium.org Link to the patchset: https://codereview.chromium.org/1766473002/#ps120001 (title: "Fixed function param docs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766473002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by btolsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766473002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Further split media_router_container tests. This change splits the media_router_container_tests.js file into two separate test files, based on test timing, to help with the timeout issue these tests have been experiencing on bots. BUG=591227 R=imcheng@chromium.org ========== to ========== [Media Router] Further split media_router_container tests. This change splits the media_router_container_tests.js file into two separate test files, based on test timing, to help with the timeout issue these tests have been experiencing on bots. BUG=591227 R=imcheng@chromium.org Committed: https://crrev.com/e144192a9b4776bc10e7c4c2dc0e369ceac763c6 Cr-Commit-Position: refs/heads/master@{#380687} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e144192a9b4776bc10e7c4c2dc0e369ceac763c6 Cr-Commit-Position: refs/heads/master@{#380687} |
