Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682313002:
Introduce NavigationSimulator to use in unit tests (Closed)
DescriptionIntroduce NavigationSimulator to use in unit tests
This CL introduces a new helper class to simulate navigations in unit tests,
the NavigationSimulator. Simulating navigations has been hard in the past, as
numerous unit tests simulate a wrong ordering of IPCs or use the wrong
RenderFrameHost to commit the navigation. Some WebContentsObserver methods may
also not be called.
The goal of the new class is to give users of the content/ API an easy way to
simulate navigations that stays up-to-date with the actual code used in
production.
BUG=688393
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2682313002
Cr-Commit-Position: refs/heads/master@{#454279}
Committed: https://chromium.googlesource.com/chromium/src/+/6b6831f2f18308600d867887809305af8642c2ed
Patch Set 1 #Patch Set 2 : Introduce NavigationSimulator #Patch Set 3 : Introduce NavigationSimulator #
Total comments: 4
Patch Set 4 : Added same-page navigation simulation #
Total comments: 33
Patch Set 5 : Rebase #Patch Set 6 : Addressed comments #Patch Set 7 : Rebase #Patch Set 8 : Fixed issue with OOPIF #
Total comments: 4
Patch Set 9 : Rebase #Patch Set 10 : Removed impl split #
Total comments: 6
Patch Set 11 : Addressed comments #Messages
Total messages: 65 (39 generated)
Description was changed from ========== Ensure ReadyToCommitNavigation is called in unit tests This CL ensures that NavigationHandle::ReadyToCommitNavigation is called by TestRenderFrameHost::SimulateNavigationCommit. BUG=688393 ========== to ========== Ensure ReadyToCommitNavigation is called in unit tests This CL ensures that NavigationHandle::ReadyToCommitNavigation is called by TestRenderFrameHost::SimulateNavigationCommit. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Ensure ReadyToCommitNavigation is called in unit tests This CL ensures that NavigationHandle::ReadyToCommitNavigation is called by TestRenderFrameHost::SimulateNavigationCommit. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Ensure ReadyToCommitNavigation is called in unit tests This CL ensures that NavigationHandle::ReadyToCommitNavigation is called by TestRenderFrameHost::SimulateNavigationCommit. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + engedy@chromium.org
@engedy: I'm trying to fix issue 688393 and have this preliminary patch. With this, all your unit tests are passing except ContentSubresourceFilterDriverFactoryTest.WhitelistSiteOnReload, and I'm not quite sure why. If you have time, could you check if you're still missing the call to ReadyToCommitNavigation with this patch, or if the problem is coming from the test harness in ContentSubresourceFilterDriverFactoryTest?
engedy@chromium.org changed reviewers: + bmcquade@chromium.org
Thanks a lot for working on this! The test `WhitelistSiteOnReload` makes decisions based on the |referrer| and the |transition_type| values it retrieves from the NavigationHandle; currently I don't see what would be the best way to set these. (Adding Bryan who added this particular test.)
On 2017/02/09 at 18:42:24, engedy wrote: > Thanks a lot for working on this! > > The test `WhitelistSiteOnReload` makes decisions based on the |referrer| and the |transition_type| values it retrieves from the NavigationHandle; currently I don't see what would be the best way to set these. > > (Adding Bryan who added this particular test.) Thanks! Yeah, this test wants to modify the referrer & transition type values returned by nav handle. It wasn't immediately clear to me how to do that. Camille, can you take a quick pass and if the fix is straightforward to you, would you be able to make the needed change? If not, I think we can disable this one test for the time being & I'll figure out how to re-enable it after this patch lands.
On 2017/02/09 19:43:48, Bryan McQuade wrote: > On 2017/02/09 at 18:42:24, engedy wrote: > > Thanks a lot for working on this! > > > > The test `WhitelistSiteOnReload` makes decisions based on the |referrer| and > the |transition_type| values it retrieves from the NavigationHandle; currently I > don't see what would be the best way to set these. > > > > (Adding Bryan who added this particular test.) > > Thanks! Yeah, this test wants to modify the referrer & transition type values > returned by nav handle. It wasn't immediately clear to me how to do that. > Camille, can you take a quick pass and if the fix is straightforward to you, > would you be able to make the needed change? If not, I think we can disable this > one test for the time being & I'll figure out how to re-enable it after this > patch lands. I see. I could add extra-parameters to the SimulatStartNavigation (and the redirect one maybe?) call where you would specify the referrer & transition values and that would probably work. But I'm thinking this may not be the right direction. Nasko and I had been talking about providing easier testing methods for navigation that accurately reflect what IPCs we should be getting, since we were worried unit tests would often try to emulate things that are out of date or just plainly wrong. The issue really seems to highlight the need for such a thing. I'm going to take a stab at creating some sort of testing class that you can use to simulate navigation in a simpler and accurate manner. If it proves too complicated I'll just add the parameters as mentioned above.
On 2017/02/10 13:14:15, clamy wrote: > On 2017/02/09 19:43:48, Bryan McQuade wrote: > > On 2017/02/09 at 18:42:24, engedy wrote: > > > Thanks a lot for working on this! > > > > > > The test `WhitelistSiteOnReload` makes decisions based on the |referrer| and > > the |transition_type| values it retrieves from the NavigationHandle; currently > I > > don't see what would be the best way to set these. > > > > > > (Adding Bryan who added this particular test.) > > > > Thanks! Yeah, this test wants to modify the referrer & transition type values > > returned by nav handle. It wasn't immediately clear to me how to do that. > > Camille, can you take a quick pass and if the fix is straightforward to you, > > would you be able to make the needed change? If not, I think we can disable > this > > one test for the time being & I'll figure out how to re-enable it after this > > patch lands. > > I see. I could add extra-parameters to the SimulatStartNavigation (and the > redirect one maybe?) call where you would specify the referrer & transition > values and that would probably work. But I'm thinking this may not be the right > direction. Nasko and I had been talking about providing easier testing methods > for navigation that accurately reflect what IPCs we should be getting, since we > were worried unit tests would often try to emulate things that are out of date > or just plainly wrong. The issue really seems to highlight the need for such a > thing. I'm going to take a stab at creating some sort of testing class that you > can use to simulate navigation in a simpler and accurate manner. If it proves > too complicated I'll just add the parameters as mentioned above. That sounds awesome! Really glad to hear about the work on easy-to-use test support to provide realistic simulation of navigations, I had often been wondering myself about how up-to-date several unittests around the codebase are.
Description was changed from ========== Ensure ReadyToCommitNavigation is called in unit tests This CL ensures that NavigationHandle::ReadyToCommitNavigation is called by TestRenderFrameHost::SimulateNavigationCommit. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce NavigationSimulator to use in unit tests This CL introduces a new helper class to simulate navigations in unit tests, the NavigationSimulator. Simulating navigations has been hard in the past, as numerous unit tests simulate a wrong ordering of IPCs or use the wrong RenderFrameHost to commit the navigation. Some WebContentsObserver methods may also not be called. The goal of the new class is to give users of the content/ API an easy way to simulate navigations that stays up-to-date with the actual code used in production. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@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 ========== Introduce NavigationSimulator to use in unit tests This CL introduces a new helper class to simulate navigations in unit tests, the NavigationSimulator. Simulating navigations has been hard in the past, as numerous unit tests simulate a wrong ordering of IPCs or use the wrong RenderFrameHost to commit the navigation. Some WebContentsObserver methods may also not be called. The goal of the new class is to give users of the content/ API an easy way to simulate navigations that stays up-to-date with the actual code used in production. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce NavigationSimulator to use in unit tests This CL introduces a new helper class to simulate navigations in unit tests, the NavigationSimulator. Simulating navigations has been hard in the past, as numerous unit tests simulate a wrong ordering of IPCs or use the wrong RenderFrameHost to commit the navigation. Some WebContentsObserver methods may also not be called. The goal of the new class is to give users of the content/ API an easy way to simulate navigations that stays up-to-date with the actual code used in production. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by clamy@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...
clamy@chromium.org changed reviewers: + nasko@chromium.org
@engedy,bmcquade: The latest CL adds a new class to simulate navigations. Does the interface look good? Note: using the class uncovered that some of your tests had issues with OOPIF. We would expect to transfer the navigation when OOPIF is enabled, and it seems the test harness of those unit tests doesn't support that very well. @nasko: PTAL. This is the class I was telling you about. Note that it does simulate navigation transfer to different RFH if needed. I've also added a lot of checks that the navigation APIs are rightly fired. This isn't needed to simulate navigations, but I'm thinking this will keep us honest about the unit test harness as we make changes to the navigation code.
thank you for this change! one request but otherwise seems good. thanks very much for the NavigationSimulator class! I've struggled to write unit tests that want to modify the nav attributes - it's really nice to have a class like NavigationSimulator to make this easier. https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (left): https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:246: if (!content::IsBrowserSideNavigationEnabled()) { the test ContentSubresourceFilterDriverFactoryTest.WhitelistSiteOnReload currently returns early if IsBrowserSideNavigationEnabled returns true. can you try removing it and see if the tests continue to pass? i'm hoping they will work with the new NavigationSimulator class you have introduced.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
This looks great! I am preparing a CL to fix these tests so they no longer want to verify things on particular RFH instances. https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:338: content::RenderFrameHostTester::For(main_rfh()) Do you think the NavigationSimulator could simulate an in-page navigation too?
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (left): https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:246: if (!content::IsBrowserSideNavigationEnabled()) { On 2017/02/14 17:26:27, Bryan McQuade wrote: > the test ContentSubresourceFilterDriverFactoryTest.WhitelistSiteOnReload > currently returns early if IsBrowserSideNavigationEnabled returns true. can you > try removing it and see if the tests continue to pass? i'm hoping they will work > with the new NavigationSimulator class you have introduced. I've re-enabled it for PlzNavigate and it seems to be working.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/40001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:338: content::RenderFrameHostTester::For(main_rfh()) On 2017/02/15 11:53:22, engedy wrote: > Do you think the NavigationSimulator could simulate an in-page navigation too? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Camille, here is the promised CL: https://codereview.chromium.org/2695243002/. We would still need to somehow expose to the tests which RFH ultimately committed the navigation.
nasko@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
Adding creis@ and nick@, who can continue the review, as I will be out next week. https://codereview.chromium.org/2682313002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:5: #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h" nit: An empty line after this header. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:25: // renderer-initiated navigation to |original_url| received by It isn't immediately clear to me what "received by" means? Is this the RFH that the navigation will end up committing? If so, how would the test code know what that would be? https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:30: Side node: It will be great to later on have static methods that implement the common patterns, such as "navigate and commit", so we can avoid boilerplate code of creating this object, starting, committing. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:78: // Simulates the commit of a same-page navigation. It will be nice to list what navigations are same-page and what it means/how it differs. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... File content/test/navigation_simulator_impl.cc (right): https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:21: class NavigationThrottleCallCounter : public NavigationThrottle { This class doesn't seem to count anything, so the name is a bit confusing. It is more of a callback runner. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:59: state_(INITIALIZATION), With the newest C++11 goodies, we can initialize these in the header file, so the initializer list can be trimmed down significantly. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:224: static_cast<TestRenderFrameHost*>(handle_->GetRenderFrameHost()); Why do we need to call this again? Wouldn't assigning it the value of new_render_frame_host be valid? https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:344: params.should_update_history = true; Is this set to true for same page? https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:345: params.did_create_new_entry = Why would we create a new entry for same page navigation? https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... File content/test/navigation_simulator_impl.h (right): https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.h:38: // WebContentsObservor: Observer, not Observor. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.h:58: // The renderer associated to this navigation. nit: "associated with"
https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:20: // renderer-initiated navigations. Should this comment, or the comment before CreateRendererInitiated, make it clear that this is for unittests, and should not be used for browsertests (since it assumes |render_frame_host| is a TestRenderFrameHost)? https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:27: static std::unique_ptr<NavigationSimulator> CreateRendererInitiated( If we want a shorter name for this, it could be FromDocument() -- and the future, browser-initiated variant being called FromBrowser. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:30: On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > Side node: It will be great to later on have static methods that implement the > common patterns, such as "navigate and commit", so we can avoid boilerplate code > of creating this object, starting, committing. An alternative to consider would be to support chaining -- the methods would "return this", so you could do one liners like: NavigationSimulator::CreateRendererInitiated(url, rfh)->Start()->Redirect(url2)->Commit(); You could also add an implicit Start() and/or Commit() in the dtor, if it hadn't been started. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:42: // simulator.Start(); These would be ->'s, not .'s, right? https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:54: // simulator.Fail(net_error); It's not clear what the identifier |net_error| indicates in this example code. Can you replace it with a constant or a literal? https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:64: virtual void Start() {} I'm curious what makes requiring Start() to be an explicit step? In what case would you want to create a NavigationSimulator and not immediately Start it?
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2682313002/diff/60001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/60001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:5: #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h" On 2017/02/16 18:47:58, nasko (out until Feb 27th) wrote: > nit: An empty line after this header. Done. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... File content/public/test/navigation_simulator.h (right): https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:20: // renderer-initiated navigations. On 2017/02/16 23:36:31, ncarter wrote: > Should this comment, or the comment before CreateRendererInitiated, make it > clear that this is for unittests, and should not be used for browsertests (since > it assumes |render_frame_host| is a TestRenderFrameHost)? Done. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:25: // renderer-initiated navigation to |original_url| received by On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > It isn't immediately clear to me what "received by" means? Is this the RFH that > the navigation will end up committing? If so, how would the test code know what > that would be? It's a renderer-initiated navigation, so it's the RFH that corresponds the RenderFrame that started it. I changed received to started, let me know if it is clearer. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:27: static std::unique_ptr<NavigationSimulator> CreateRendererInitiated( On 2017/02/16 23:36:31, ncarter wrote: > If we want a shorter name for this, it could be FromDocument() -- and the > future, browser-initiated variant being called FromBrowser. I'm not sure it's clearer in that case, but I've used it in the new static functions, whose names were a bit long otherwise. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:30: On 2017/02/16 23:36:31, ncarter wrote: > On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > > Side node: It will be great to later on have static methods that implement the > > common patterns, such as "navigate and commit", so we can avoid boilerplate > code > > of creating this object, starting, committing. > > An alternative to consider would be to support chaining -- the methods would > "return this", so you could do one liners like: > > NavigationSimulator::CreateRendererInitiated(url, > rfh)->Start()->Redirect(url2)->Commit(); > > You could also add an implicit Start() and/or Commit() in the dtor, if it hadn't > been started. I have updated the CL so that methods call any earlier omitted methods. I think this should make it easier. I have added a static method for navigate and commit or navigate and fail as well. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:42: // simulator.Start(); On 2017/02/16 23:36:32, ncarter wrote: > These would be ->'s, not .'s, right? Done. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:54: // simulator.Fail(net_error); On 2017/02/16 23:36:31, ncarter wrote: > It's not clear what the identifier |net_error| indicates in this example code. > Can you replace it with a constant or a literal? Done. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:64: virtual void Start() {} On 2017/02/16 23:36:32, ncarter wrote: > I'm curious what makes requiring Start() to be an explicit step? In what case > would you want to create a NavigationSimulator and not immediately Start it? The goal is for people to create the NavigationSimulator, set up some parameters (referrer, transition, etc...) and then start it once all parameters are correct. I could have done that in the constructor, but I wanted to avoid having many arguments in the constructor that would be useless for most users. https://codereview.chromium.org/2682313002/diff/60001/content/public/test/nav... content/public/test/navigation_simulator.h:78: // Simulates the commit of a same-page navigation. On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > It will be nice to list what navigations are same-page and what it means/how it > differs. Done. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... File content/test/navigation_simulator_impl.cc (right): https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:21: class NavigationThrottleCallCounter : public NavigationThrottle { On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > This class doesn't seem to count anything, so the name is a bit confusing. It is > more of a callback runner. Indeed. Renamed it. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:59: state_(INITIALIZATION), On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > With the newest C++11 goodies, we can initialize these in the header file, so > the initializer list can be trimmed down significantly. Done. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:224: static_cast<TestRenderFrameHost*>(handle_->GetRenderFrameHost()); On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > Why do we need to call this again? Wouldn't assigning it the value of > new_render_frame_host be valid? Done. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:344: params.should_update_history = true; On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > Is this set to true for same page? I think so. See https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_co... for an example of a unit tests with an in page navigation where this is set to true. I think it's always supposed to be true, except in specific cases like unreachable url. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.cc:345: params.did_create_new_entry = On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > Why would we create a new entry for same page navigation? Done. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... File content/test/navigation_simulator_impl.h (right): https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.h:38: // WebContentsObservor: On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > Observer, not Observor. Done. https://codereview.chromium.org/2682313002/diff/60001/content/test/navigation... content/test/navigation_simulator_impl.h:58: // The renderer associated to this navigation. On 2017/02/16 18:47:59, nasko (out until Feb 27th) wrote: > nit: "associated with" Done.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by clamy@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.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
I have a bunch of CLs dependent on this test infrastructure. nick@, friendly ping?
On 2017/02/23 01:28:15, Charlie Harrison wrote: > I have a bunch of CLs dependent on this test infrastructure. nick@, friendly > ping? acking this ping -- will review asap, it had fallen off my radar. sorry!
lgtm https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... File content/test/navigation_simulator_impl.h (right): https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... content/test/navigation_simulator_impl.h:21: class NavigationSimulatorImpl : public NavigationSimulator, It looks like this ought to be preferred over RenderFrameHostTester::For(rfh)->SimulateNavigationStart, etc. Should we put a comment in the render_frame_host_tester.h saying that those methods are deprecated? https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... content/test/navigation_simulator_impl.h:22: WebContentsObserver { should be public WebContentsObserver
LGTM, thanks!
The CQ bit was checked by clamy@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...
@engedy: PTAL at the changes in components/subresource_filter @nick: following offline drive-by comment from jam@, I've removed the impl split of NavigationSimulator. The implementation now lives directly in the content/public/test files, since this is allowed for content/public/test. Is it still good for you? https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... File content/test/navigation_simulator_impl.h (right): https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... content/test/navigation_simulator_impl.h:21: class NavigationSimulatorImpl : public NavigationSimulator, On 2017/02/28 23:27:44, ncarter wrote: > It looks like this ought to be preferred over > RenderFrameHostTester::For(rfh)->SimulateNavigationStart, etc. Should we put a > comment in the render_frame_host_tester.h saying that those methods are > deprecated? Done. https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... content/test/navigation_simulator_impl.h:22: WebContentsObserver { On 2017/02/28 23:27:44, ncarter wrote: > should be public WebContentsObserver Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/01 16:18:09, clamy wrote: > @engedy: PTAL at the changes in components/subresource_filter > @nick: following offline drive-by comment from jam@, I've removed the impl split > of NavigationSimulator. The implementation now lives directly in the > content/public/test files, since this is allowed for content/public/test. Is it > still good for you? Yes, that's an improvement; makes it easier to understand. > https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... > File content/test/navigation_simulator_impl.h (right): > > https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... > content/test/navigation_simulator_impl.h:21: class NavigationSimulatorImpl : > public NavigationSimulator, > On 2017/02/28 23:27:44, ncarter wrote: > > It looks like this ought to be preferred over > > RenderFrameHostTester::For(rfh)->SimulateNavigationStart, etc. Should we put a > > comment in the render_frame_host_tester.h saying that those methods are > > deprecated? > > Done. > > https://codereview.chromium.org/2682313002/diff/140001/content/test/navigatio... > content/test/navigation_simulator_impl.h:22: WebContentsObserver { > On 2017/02/28 23:27:44, ncarter wrote: > > should be public WebContentsObserver > > Done.
components/subresource_filter LGTM % nits. Thanks a lot for working on NavigationSimulator, it greatly increases the value and trustworthiness of unittests such as this! https://codereview.chromium.org/2682313002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (left): https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:17: #include "content/public/common/browser_side_navigation_policy.h" nit: No longer needed. https://codereview.chromium.org/2682313002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:287: // Update the subframe RFH as the navigation may have lead to a process nit: Given that we need to go hunting for this subframe here anyway, how about getting rid of |subframe_rfh_| entirely, and moving this block to become the implementation of subframe_rfh()? (Alternatively, NavigationSimulator could expose the RFH the simulated navigation landed in.) https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:340: // ReadyToCommitNavigation with browser-side navigation disabled is not nit: Update this comment, suggestion: // With browser-side navigation enabled, ReadyToCommitNavigation is invoked even for failed navigations. This is correctly simulated by NavigationSimulator. Make sure no activation message is sent in this case.
Thanks! https://codereview.chromium.org/2682313002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (left): https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:17: #include "content/public/common/browser_side_navigation_policy.h" On 2017/03/02 10:38:10, engedy (slow) wrote: > nit: No longer needed. Done. https://codereview.chromium.org/2682313002/diff/180001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:287: // Update the subframe RFH as the navigation may have lead to a process On 2017/03/02 10:38:10, engedy (slow) wrote: > nit: Given that we need to go hunting for this subframe here anyway, how about > getting rid of |subframe_rfh_| entirely, and moving this block to become the > implementation of subframe_rfh()? > > (Alternatively, NavigationSimulator could expose the RFH the simulated > navigation landed in.) I'll go with moving this block. We could have NavigationSimulator expose it eventually. I'd like to land this ASAP so I'll avoid any modification to the NavigationSimulator code right now. https://codereview.chromium.org/2682313002/diff/180001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:340: // ReadyToCommitNavigation with browser-side navigation disabled is not On 2017/03/02 10:38:10, engedy (slow) wrote: > nit: Update this comment, suggestion: > > // With browser-side navigation enabled, ReadyToCommitNavigation is invoked even > for failed navigations. This is correctly simulated by NavigationSimulator. Make > sure no activation message is sent in this case. Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, bmcquade@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2682313002/#ps200001 (title: "Addressed comments")
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
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 clamy@chromium.org
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": 200001, "attempt_start_ts": 1488469571032750,
"parent_rev": "5bcd48909cf0f27e9eff04f904fd0ba83292edf9", "commit_rev":
"6b6831f2f18308600d867887809305af8642c2ed"}
Message was sent while issue was closed.
Description was changed from ========== Introduce NavigationSimulator to use in unit tests This CL introduces a new helper class to simulate navigations in unit tests, the NavigationSimulator. Simulating navigations has been hard in the past, as numerous unit tests simulate a wrong ordering of IPCs or use the wrong RenderFrameHost to commit the navigation. Some WebContentsObserver methods may also not be called. The goal of the new class is to give users of the content/ API an easy way to simulate navigations that stays up-to-date with the actual code used in production. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Introduce NavigationSimulator to use in unit tests This CL introduces a new helper class to simulate navigations in unit tests, the NavigationSimulator. Simulating navigations has been hard in the past, as numerous unit tests simulate a wrong ordering of IPCs or use the wrong RenderFrameHost to commit the navigation. Some WebContentsObserver methods may also not be called. The goal of the new class is to give users of the content/ API an easy way to simulate navigations that stays up-to-date with the actual code used in production. BUG=688393 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2682313002 Cr-Commit-Position: refs/heads/master@{#454279} Committed: https://chromium.googlesource.com/chromium/src/+/6b6831f2f18308600d8678878093... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/6b6831f2f18308600d8678878093... |
