Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2470413004:
PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests (Closed)
DescriptionPlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests
The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation
code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation
is not instantiated. The navigation code in RenderFrame checks for whether browser side
navigation is enabled and sends IPCs etc which don't work for the tests as written.
Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being
exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation
IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which
is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function
CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread
which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests.
The other alternative was to make these tests full fledged browser tests which would be a touch
difficult given the way these tests have been written.
BUG=662268
TBR=jam
Committed: https://crrev.com/5fc8d8d462127a0f20651df4c8a8971926f21f50
Cr-Commit-Position: refs/heads/master@{#430811}
Patch Set 1 #Patch Set 2 : Update DEPS #Patch Set 3 : Revert some changes #Patch Set 4 : Revert more changes #Patch Set 5 : Fix build redness #
Total comments: 6
Patch Set 6 : Address review comments #
Messages
Total messages: 37 (22 generated)
ananta@chromium.org changed reviewers: + jam@chromium.org
The CQ bit was checked by ananta@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.
It seems either we need to: 1) figure out a way to fake whatever navigation-related functionality is missing with that test harness 2) rewrite the test as a browsertest 3) refactor the code in the renderer so that we can test it in an isolated way that doesn't depend on whether plznavigate is enabled or not I personally prefer 3, but should also get the input of the team who wrote these tests. but i don't think we should disable them this way; what's the advantage of doing so vs keeping them in the filter list?
Description was changed from ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. With the tests as written, it is best to provide a way to turn off PlzNavigate for tests based on a global flag. For this I added the following method to content\public\common SetBrowserSideNavigationEnabledForTests which sets a global flag which defaults to true. PlzNavigate is deemed as enabled only when the global flag and the command line both are on. BUG=662268 ========== to ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 ==========
On 2016/11/04 16:18:52, jam (ooo until Dec 5) wrote: > It seems either we need to: > 1) figure out a way to fake whatever navigation-related functionality is missing > with that test harness > 2) rewrite the test as a browsertest > 3) refactor the code in the renderer so that we can test it in an isolated way > that doesn't depend on whether plznavigate is enabled or not > > I personally prefer 3, but should also get the input of the team who wrote these > tests. > > but i don't think we should disable them this way; what's the advantage of doing > so vs keeping them in the filter list? Added a way to mock out FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs which are sent in the codepaths exercised in these tests. Most of the changes are related to that along with a couple of DEPS changes.
ananta@chromium.org changed reviewers: + nparker@chromium.org
The CQ bit was checked by ananta@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_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ananta@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc (right): https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:217: message.reset(new FrameMsg_CommitNavigation( I'm not familiar with these IPCs to verify the correctness of this code. I'm assuming that the test runs and passes as before, yes? https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:263: ChromeMockRenderThread* CreateMockRenderThread() override { Add a comment as to which class this is overriding. https://codereview.chromium.org/2470413004/diff/80001/chrome/test/base/chrome... File chrome/test/base/chrome_render_view_test.h (right): https://codereview.chromium.org/2470413004/diff/80001/chrome/test/base/chrome... chrome/test/base/chrome_render_view_test.h:51: // this method and return a subclass of ChromeMockRenderThread. nit: mention who owns this naked ptr.
https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc (right): https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:217: message.reset(new FrameMsg_CommitNavigation( On 2016/11/08 23:27:30, Nathan Parker wrote: > I'm not familiar with these IPCs to verify the correctness of this code. I'm > assuming that the test runs and passes as before, yes? Yes. The BeginNavigation IPC is sent by the renderer to initiate a navigation with PlzNavigate. The browser responds with a CommitNavigation IPC with the response stream. The renderer proceeds to load the request. Here we shortcircuit the process and return the mocked IPC back. https://codereview.chromium.org/2470413004/diff/80001/chrome/renderer/safe_br... chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:263: ChromeMockRenderThread* CreateMockRenderThread() override { On 2016/11/08 23:27:30, Nathan Parker wrote: > Add a comment as to which class this is overriding. Done. https://codereview.chromium.org/2470413004/diff/80001/chrome/test/base/chrome... File chrome/test/base/chrome_render_view_test.h (right): https://codereview.chromium.org/2470413004/diff/80001/chrome/test/base/chrome... chrome/test/base/chrome_render_view_test.h:51: // this method and return a subclass of ChromeMockRenderThread. On 2016/11/08 23:27:30, Nathan Parker wrote: > nit: mention who owns this naked ptr. Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2470413004/#ps100001 (title: "Address review 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 ========== to ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 TBR=jam ==========
TBR'ing jam
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 TBR=jam ========== to ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 TBR=jam ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 TBR=jam ========== to ========== PlzNavigate: Fix for the PhishingDOMFeatureExtractorTest.SubFrames and SubframeRemoval tests The PhishingDOMFeatureExtractorTest tests instantiate the renderer side of the navigation code namely RenderFrameImpl, RenderView, etc. However the browser side of the navigation is not instantiated. The navigation code in RenderFrame checks for whether browser side navigation is enabled and sends IPCs etc which don't work for the tests as written. Proposed fix is to provide a way to mock the IPCs being sent in the codepaths being exercised in these tests. Specifically the FrameHostMsg_BeginNavigation and FrameMsg_CommitNavigation IPCs. To achieve this we provide a way to subclass the ChromeMockRenderThread class which is instantiated by the ChromeRenderViewTest class. This is achieved via a virtual function CreateMockRenderThread(). This function is overridden by a new class PhishingMockRenderThread which is instantiated for the duration of the PhishingDOMFeatureExtractorTest.* tests. The other alternative was to make these tests full fledged browser tests which would be a touch difficult given the way these tests have been written. BUG=662268 TBR=jam Committed: https://crrev.com/5fc8d8d462127a0f20651df4c8a8971926f21f50 Cr-Commit-Position: refs/heads/master@{#430811} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5fc8d8d462127a0f20651df4c8a8971926f21f50 Cr-Commit-Position: refs/heads/master@{#430811}
Message was sent while issue was closed.
not lgtm, the deps line is not ok. let's discuss a way to fix this without the deps exclusions for future, please don't TBR changes like this, see https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...
Message was sent while issue was closed.
On 2016/12/06 15:26:11, John Abd-El-Malek wrote: > not lgtm, the deps line is not ok. > > let's discuss a way to fix this without the deps exclusions > > for future, please don't TBR changes like this, see > https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review... Sure thing. Will send out a followup patch fixing this. I assumed that the DEPS rules were ok because it was in a test.
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2565543006/ by jam@chromium.org. The reason for reverting is: This isn't needed anymore after r437654. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
