Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634213002:
[Media Router] Add integration browser tests (Closed)
Description[Media Router] Add integration browser tests
This CL adds the following tests:
- MediaRouterIntegrationBrowserTest.MANUAL_Fail_SendMessage
- MediaRouterIntegrationIncognitoBrowserTest.MANUAL_Basic
- MediaRouterIntegrationIncognitoBrowserTest.MANUAL_ReconnectSession
- (Rename MANUAL_OnClose to MANUAL_CloseOnError)
and does some refactoring to factor out parts common across many tests.
BUG=678472
Review-Url: https://codereview.chromium.org/2634213002
Cr-Commit-Position: refs/heads/master@{#451492}
Committed: https://chromium.googlesource.com/chromium/src/+/ad5a427bf19d3422f39d4d4f2b0b6ad97621c8a6
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rebase #Patch Set 3 : Address Mark's comments #
Total comments: 20
Patch Set 4 : Address Mark's comments #Patch Set 5 : Add a comment, remove CloseAndReconnect #Patch Set 6 : Undo renaming waitUntilDeviceAvailable #
Messages
Total messages: 49 (39 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [Media Router] Add integration browser tests BUG= ========== to ========== [Media Router] Add integration browser tests This CL adds the following tests: - MANUAL_IncognitoBasic - MANUAL_CloseAndReconnect - MANUAL_Fail_SendMessage - MANUAL_IncognitoReconnectSession - (Rename MANUAL_OnClose to MANUAL_CloseOnError) and does some refactoring to factor out parts common across many tests. BUG=678472 ==========
Patchset #1 (id:1) has been deleted
takumif@chromium.org changed reviewers: + mfoltz@chromium.org
Please take a look, thanks!
mfoltz@chromium.org changed reviewers: + cliffordcheng@chromium.org
Thanks for adding browser test coverage! A couple of points: - A lot of the test cases have repeated blocks code to navigate a tab and do something to it (returning the resulting WebContents). Anything you can do to factor out common cases with a more descriptive API would be helpful. - This folder need a README.md, It's not clear to me what the difference is between a e2e, integration or UI browser test are or where new test cases should go. If you want to take a stab, go ahead. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_base_browsertest.cc:72: LoadExtensionIncognito(extension_unpacked_); What if is_unpacked() == false? https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_base_browsertest.cc:83: NOTIMPLEMENTED(); Can we just assume is_unpacked() is always true? (I'm not familiar with this class specifically or why it needs to handle packed and unpacked extensions.) https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_base_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_base_browsertest.h:48: Browser* GetIncognitoBrowserWithMRExtension(); It seems inconsistent for this to return a Browser* while InstallAndEnableMRExtension() below for normal profiles does not. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:492: content::WebContents* new_web_contents = This is repeated throughout the code. Add a GetActiveWebContents() method to the test class? https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:543: CheckSessionValidity(web_contents); Why not have StartSessionWithTestPageAndChooseSink() return the active web contents after checking its "session validity"? https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.h:79: void StartSessionWithTestPage(Browser* browser); Here's a thought. We could run all the test cases with a normal profile and an incognito profile. Would it be possible to create two subclasses of a shared test class that defines the tests? Each subclass would set browser() differently and run the same list of test cases. I looked through the googletest documentation and didn't see a quick way to run the same tests twice with two different test classes, other than manually defining two lists of test cases that each invoke a test method declared in the base class. Maybe there's a macro solution? https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/resources/common.js:187: * thrown. Requires |startedConnection.state| to not be "connected." Can you pass in an expectation of the connection state and verify it in the function?
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
takumif@chromium.org changed reviewers: + leilei@chromium.org - cliffordcheng@chromium.org
I've tried factoring out more of the common parts. For incognito tests, I made a subclass of MediaRouterIntegrationBrowserTests that overrides the necessary parts. I'm not sure if this was the best way to do it, but other ways seemed to require bigger changes. I didn't make the incognito class run more of the tests, since many of them were failing for various reasons and will need to be looked at more. Lei mentioned that it would be nice if MediaRouterE2EBrowserTest could inherit from MediaRouterBaseBrowserTest instead of MediaRouterIntegrationBrowserTest, but that'd be a big change, so I won't be doing that in this CL. Please take a look, thanks! https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_base_browsertest.cc:83: NOTIMPLEMENTED(); On 2017/01/20 19:00:09, mark a. foltz wrote: > Can we just assume is_unpacked() is always true? > > (I'm not familiar with this class specifically or why it needs to handle packed > and unpacked extensions.) Lei said there were plans to support packaged extension but it's not happening, so it's okay to remove it. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_base_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_base_browsertest.h:48: Browser* GetIncognitoBrowserWithMRExtension(); On 2017/01/20 19:00:09, mark a. foltz wrote: > It seems inconsistent for this to return a Browser* while > InstallAndEnableMRExtension() below for normal profiles does not. Making a subclass for incognito. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:492: content::WebContents* new_web_contents = On 2017/01/20 19:00:09, mark a. foltz wrote: > This is repeated throughout the code. Add a GetActiveWebContents() method to > the test class? Done. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.cc:543: CheckSessionValidity(web_contents); On 2017/01/20 19:00:09, mark a. foltz wrote: > Why not have StartSessionWithTestPageAndChooseSink() return the active web > contents after checking its "session validity"? Done. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/media_router_integration_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/media_router_integration_browsertest.h:79: void StartSessionWithTestPage(Browser* browser); On 2017/01/20 19:00:09, mark a. foltz wrote: > Here's a thought. We could run all the test cases with a normal profile and an > incognito profile. > > Would it be possible to create two subclasses of a shared test class that > defines the tests? Each subclass would set browser() differently and run the > same list of test cases. > > I looked through the googletest documentation and didn't see a quick way to run > the same tests twice with two different test classes, other than manually > defining two lists of test cases that each invoke a test method declared in the > base class. Maybe there's a macro solution? I made a subclass of MediaRouterIntegrationBrowserTest for incognito. I wonder if there is a cleaner way to do this. I haven't made the incognito class to run all the tests since many of the tests were failing for various reasons. https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/40001/chrome/test/media_route... chrome/test/media_router/resources/common.js:187: * thrown. Requires |startedConnection.state| to not be "connected." On 2017/01/20 19:00:09, mark a. foltz wrote: > Can you pass in an expectation of the connection state and verify it in the > function? Done.
LGTM with comments addressed This is a vast improvement! Thanks for taking the extra time to refactor this code. My comments are questions that might require a clarifying comment, and some naming/method splitting suggestions. In general I prefer explicit methods versus a single method that has two functions depending on parameters. Here's a more severe illustration of what I'm talking about. https://agiletribe.wordpress.com/2011/11/02/22-avoid-multi-purpose-methods/ https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_base_browsertest.cc:132: return ExtensionBrowserTest::browser(); If this method returns a Browser*, there's no need to override it from the superclass. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.cc:169: ExecuteJavaScriptAPI(web_contents, kWaitDeviceScript); I feel link this should be kWaitSinkScript to be consistent with other script names. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.cc:424: void MediaRouterIntegrationBrowserTest::WaitUntilSinkDiscoveredOnUI() { How is this different from running kWaitDeviceScript? https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_integration_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:79: content::WebContents* StartSessionWithTestPage(bool wait_for_sink); Slight preference for having explicit functions StartSessionWithTestPageNow() and StartSessionWithTestPageAndSink() If it doesn't result in excessive code duplication in the implementation. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:84: content::WebContents* StartSessionWithTestPageAndChooseSink( Maybe the caller could choose to call CheckSessionValidity() instead of passing a should_succeed parameter here? Or are they checking different things? https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:144: void HideMediaRouterDialog(content::WebContents* web_contents, HideMediaRouterDialogNow() and HideMediaRouterDialogWithSink() vs. wait_for_sink https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:182: Browser* browser_ = nullptr; Does this hide a member in the superclass? Maybe name it incognito_browser_ to avoid any name collisions. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/resources/common.js:146: function closeSessionAndWaitForStateChange() { closeConnectionAndWaitForStateChange
Thanks for reviewing! https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_base_browsertest.cc:132: return ExtensionBrowserTest::browser(); On 2017/02/09 23:29:58, mark a. foltz wrote: > If this method returns a Browser*, there's no need to override it from the > superclass. The superclass' browser() is not virtual, and if I want to make MediaRouterBaseBrowserTest::browser() virtual so that it can be overridden by the incognito class, I need to define it here like this. Maybe there is a better way to do this? https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_integration_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.cc:169: ExecuteJavaScriptAPI(web_contents, kWaitDeviceScript); On 2017/02/09 23:29:59, mark a. foltz wrote: > I feel link this should be kWaitSinkScript to be consistent with other script > names. Done. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.cc:424: void MediaRouterIntegrationBrowserTest::WaitUntilSinkDiscoveredOnUI() { On 2017/02/09 23:29:59, mark a. foltz wrote: > How is this different from running kWaitDeviceScript? This waits for a sink to appear in the dialog UI, whereas kWaitDeviceScript waits for PresentationRequest.getAvailability() promise to be resolved. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_integration_browsertest.h (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:79: content::WebContents* StartSessionWithTestPage(bool wait_for_sink); On 2017/02/09 23:29:59, mark a. foltz wrote: > Slight preference for having explicit functions StartSessionWithTestPageNow() > and StartSessionWithTestPageAndSink() > > If it doesn't result in excessive code duplication in the implementation. It does result in a bit of code duplication, but the more descriptive method names are probably better. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:84: content::WebContents* StartSessionWithTestPageAndChooseSink( On 2017/02/09 23:29:59, mark a. foltz wrote: > Maybe the caller could choose to call CheckSessionValidity() instead of passing > a should_succeed parameter here? Or are they checking different things? Done. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:144: void HideMediaRouterDialog(content::WebContents* web_contents, On 2017/02/09 23:29:59, mark a. foltz wrote: > HideMediaRouterDialogNow() and HideMediaRouterDialogWithSink() vs. wait_for_sink Factored out the common part into GetControllerForShownDialog() and got rid of HideMediaRouterDialog(). https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_integration_browsertest.h:182: Browser* browser_ = nullptr; On 2017/02/09 23:29:59, mark a. foltz wrote: > Does this hide a member in the superclass? Maybe name it incognito_browser_ to > avoid any name collisions. Done. Also renaming extension_id_ to incognito_extension_id_. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/resources/common.js:146: function closeSessionAndWaitForStateChange() { On 2017/02/09 23:29:59, mark a. foltz wrote: > closeConnectionAndWaitForStateChange Done. Should all uses of "session" in this file be replaced with "connection"? I'm not sure what a session is in the context of Presentation API.
lgtm
LGTM https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_base_browsertest.cc:132: return ExtensionBrowserTest::browser(); On 2017/02/14 at 19:11:48, takumif wrote: > On 2017/02/09 23:29:58, mark a. foltz wrote: > > If this method returns a Browser*, there's no need to override it from the > > superclass. > > The superclass' browser() is not virtual, and if I want to make MediaRouterBaseBrowserTest::browser() virtual so that it can be overridden by the incognito class, I need to define it here like this. Maybe there is a better way to do this? It's a little weird to override a non-virtual method with a virtual one. But I guess it's legal, and the style guide doesn't prohibit it, so I guess it's okay. Please add a note explaining why you're doing this. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/resources/common.js:146: function closeSessionAndWaitForStateChange() { On 2017/02/14 at 19:11:48, takumif wrote: > On 2017/02/09 23:29:59, mark a. foltz wrote: > > closeConnectionAndWaitForStateChange > > Done. Should all uses of "session" in this file be replaced with "connection"? I'm not sure what a session is in the context of Presentation API. Ideally they should, but don't worry about mass renamings here. That can be done with a separate and mechanical change.
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Media Router] Add integration browser tests This CL adds the following tests: - MANUAL_IncognitoBasic - MANUAL_CloseAndReconnect - MANUAL_Fail_SendMessage - MANUAL_IncognitoReconnectSession - (Rename MANUAL_OnClose to MANUAL_CloseOnError) and does some refactoring to factor out parts common across many tests. BUG=678472 ========== to ========== [Media Router] Add integration browser tests This CL adds the following tests: - MediaRouterIntegrationBrowserTest.MANUAL_Fail_SendMessage - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_Basic - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_ReconnectSession - (Rename MANUAL_OnClose to MANUAL_CloseOnError) and does some refactoring to factor out parts common across many tests. BUG=678472 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by takumif@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Removing the test for PresentationConnection.reconnect() until that's implemented. Also delaying the rename from device->sink and session->connection, since that'd require changes in Java code as well. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/media_router_base_browsertest.cc (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/media_router_base_browsertest.cc:132: return ExtensionBrowserTest::browser(); On 2017/02/15 00:41:27, mark a. foltz wrote: > On 2017/02/14 at 19:11:48, takumif wrote: > > On 2017/02/09 23:29:58, mark a. foltz wrote: > > > If this method returns a Browser*, there's no need to override it from the > > > superclass. > > > > The superclass' browser() is not virtual, and if I want to make > MediaRouterBaseBrowserTest::browser() virtual so that it can be overridden by > the incognito class, I need to define it here like this. Maybe there is a better > way to do this? > > It's a little weird to override a non-virtual method with a virtual one. But I > guess it's legal, and the style guide doesn't prohibit it, so I guess it's okay. > Please add a note explaining why you're doing this. Done. https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2634213002/diff/160001/chrome/test/media_rout... chrome/test/media_router/resources/common.js:146: function closeSessionAndWaitForStateChange() { On 2017/02/15 00:41:27, mark a. foltz wrote: > On 2017/02/14 at 19:11:48, takumif wrote: > > On 2017/02/09 23:29:59, mark a. foltz wrote: > > > closeConnectionAndWaitForStateChange > > > > Done. Should all uses of "session" in this file be replaced with "connection"? > I'm not sure what a session is in the context of Presentation API. > > Ideally they should, but don't worry about mass renamings here. That can be > done with a separate and mechanical change. Acknowledged.
The CQ bit was checked by takumif@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, leilei@chromium.org Link to the patchset: https://codereview.chromium.org/2634213002/#ps220001 (title: "Undo renaming waitUntilDeviceAvailable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487450652589830,
"parent_rev": "616e7d26f65d25ccb181c5b2956a73f4da67baa7", "commit_rev":
"ad5a427bf19d3422f39d4d4f2b0b6ad97621c8a6"}
Message was sent while issue was closed.
Description was changed from ========== [Media Router] Add integration browser tests This CL adds the following tests: - MediaRouterIntegrationBrowserTest.MANUAL_Fail_SendMessage - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_Basic - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_ReconnectSession - (Rename MANUAL_OnClose to MANUAL_CloseOnError) and does some refactoring to factor out parts common across many tests. BUG=678472 ========== to ========== [Media Router] Add integration browser tests This CL adds the following tests: - MediaRouterIntegrationBrowserTest.MANUAL_Fail_SendMessage - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_Basic - MediaRouterIntegrationIncognitoBrowserTest.MANUAL_ReconnectSession - (Rename MANUAL_OnClose to MANUAL_CloseOnError) and does some refactoring to factor out parts common across many tests. BUG=678472 Review-Url: https://codereview.chromium.org/2634213002 Cr-Commit-Position: refs/heads/master@{#451492} Committed: https://chromium.googlesource.com/chromium/src/+/ad5a427bf19d3422f39d4d4f2b0b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/ad5a427bf19d3422f39d4d4f2b0b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
