Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2065733002:
Add a method to override the network conditions of the ChromeDriver session. (Closed)
Description[chromedriver] Add a method to override the network conditions of the ChromeDriver session.
Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline.
BUG=chromedriver:984
Committed: https://crrev.com/17a2e1eae574d917f9ccb1eeccf8f11426ac5588
Cr-Commit-Position: refs/heads/master@{#403209}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Test XHR with setting network emulation and refactored netowrk setter method #
Total comments: 20
Patch Set 3 : Changed hard coded network values to constants. Fixed style issues. #
Total comments: 37
Patch Set 4 : Added network connection tests, fixed WebView loop errors #Patch Set 5 : Added field in ChromeDesktopImpl to store value of the user's connection type. #
Total comments: 24
Patch Set 6 : Changed web_view loop in SetNetworkConnection. Fixed function names in ChromeDesktopImpl. #
Total comments: 18
Patch Set 7 : Fixed style issues. Changed names for consistency. #
Total comments: 20
Patch Set 8 : Fixed spacing, formatting and names. Deleted unused test code. #Patch Set 9 : Moved fuction to top of setNetworkConnection #Messages
Total messages: 32 (9 generated)
Description was changed from ========== Added a method to set the overridden network conditionsin the session. Added a test for the method, and another test to look at network conditions between different windows. BUG= ========== to ========== Added a method to set the overridden network conditionsin the session. Added a test for the method, and another test to look at network conditions between different windows. BUG= ==========
roisinmcl@google.com changed reviewers: + samuong@google.com
samuong@chromium.org changed reviewers: + samuong@chromium.org
https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/network_list.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/network_list.cc:25: "\"latency\": 0}]"; nitpick: since this isn't a real change, let's remove this from the cl - let me know if you need help doing this in git https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/chromedriver.py:416: 'connection_type': connection_type i think the spec says this should look like: params = { 'parameters': { 'type': connectionType } } https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... File chrome/test/chromedriver/client/command_executor.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/command_executor.py:149: GET_EMULATED_NETWORK_CONDITIONS = (_Method.GET, '/session/:sessionId/network_connection') style nitpick: keep lines to within 80 charws https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/command_executor.py:151: NETWORK_CONDITIONS_DIFFERENT_WEBVIEWS = (_Method.GET, 'session/:sessionId/network_connection') lines 149 and 151 are the same commmand (same http method and endpoint) so the second one is not necessary https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/se... File chrome/test/chromedriver/server/http_handler.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/se... chrome/test/chromedriver/server/http_handler.cc:388: base::Bind(&ExecuteSetEmulatedNetworkConditions))), style nitpicks: unindent so that the second param is aligned with the first, and don't exceed 80 chars in a line https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/te... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/te... chrome/test/chromedriver/test/run_py_tests.py:1031: nitpick: this blank line is not needed https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:12: #include "base/json/json_reader.h" is this needed? https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:30: #include "chrome/test/chromedriver/chrome/network_list.h" is this for the bitmasks? https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:997: nitpick: delete this blank line https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1044: Status ExecuteSetEmulatedNetworkConditions(Session* session, i took a closer look into this and although you're right that it does get applied to all tabs, i still think this should be a session command. the reason is that it won't get applied to all tabs at the same time, and certain network accesses (XHRs) won't get emulated correctly. we might have this go through all of the WebViews for the session, and call OverrideNetworkConditions on each one. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1048: Timeout* timeout) { style nitpick: keep all parameters indented to the same level https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1049: const int kOfflineMask = 0x0; is this supposed to be 0x1? https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1057: if(!params.GetInteger("connection_type", &connection_type)) { i think this is called "parameters.type" in the spec https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1065: if (connection_type == kOfflineMask) { use the bitwise and operator (&) instead of == to test for bits. in many cases, multiple bits will be set (e.g. airplane mode + wifi) so you'll also need to be careful about the order of these if statements. in the airplane mode + wifi example, wifi should take precedence. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1092: "invalid 'connection_type'"); style nitpick: join the two lines above https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.h (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.h:312: Status ExecuteSetEmulatedNetworkConditions(Session* session, this is a really long name, let's change this to ExecuteSetNetworkConnection, and be consistent with the mobile spec this change might need to be made in a few more places which have similarly named functions (e.g. in command_executor.py)
https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/chromedriver.py:416: 'connection_type': connection_type On 2016/06/14 17:45:59, samuong wrote: > i think the spec says this should look like: > > params = { > 'parameters': { > 'type': connectionType > } > } Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... File chrome/test/chromedriver/client/command_executor.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/command_executor.py:149: GET_EMULATED_NETWORK_CONDITIONS = (_Method.GET, '/session/:sessionId/network_connection') On 2016/06/14 17:45:59, samuong wrote: > style nitpick: keep lines to within 80 charws Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/cl... chrome/test/chromedriver/client/command_executor.py:151: NETWORK_CONDITIONS_DIFFERENT_WEBVIEWS = (_Method.GET, 'session/:sessionId/network_connection') On 2016/06/14 17:45:59, samuong wrote: > lines 149 and 151 are the same commmand (same http method and endpoint) so the > second one is not necessary Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/te... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/te... chrome/test/chromedriver/test/run_py_tests.py:1031: On 2016/06/14 17:46:00, samuong wrote: > nitpick: this blank line is not needed Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:12: #include "base/json/json_reader.h" On 2016/06/14 17:46:00, samuong wrote: > is this needed? I have removed this. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:30: #include "chrome/test/chromedriver/chrome/network_list.h" On 2016/06/14 17:46:00, samuong wrote: > is this for the bitmasks? I removed this and declared the bitmasks within the function. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:997: On 2016/06/14 17:46:00, samuong wrote: > nitpick: delete this blank line Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1044: Status ExecuteSetEmulatedNetworkConditions(Session* session, On 2016/06/14 17:46:00, samuong wrote: > i took a closer look into this and although you're right that it does get > applied to all tabs, i still think this should be a session command. the reason > is that it won't get applied to all tabs at the same time, and certain network > accesses (XHRs) won't get emulated correctly. > > we might have this go through all of the WebViews for the session, and call > OverrideNetworkConditions on each one. Moved this function to session_commands and iterated through the WebViews and called OverrideNetworkConditions on each one. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1048: Timeout* timeout) { On 2016/06/14 17:46:00, samuong wrote: > style nitpick: keep all parameters indented to the same level Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1049: const int kOfflineMask = 0x0; On 2016/06/14 17:46:00, samuong wrote: > is this supposed to be 0x1? Changed this to 0x1. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1057: if(!params.GetInteger("connection_type", &connection_type)) { On 2016/06/14 17:46:00, samuong wrote: > i think this is called "parameters.type" in the spec Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1065: if (connection_type == kOfflineMask) { On 2016/06/14 17:46:00, samuong wrote: > use the bitwise and operator (&) instead of == to test for bits. in many cases, > multiple bits will be set (e.g. airplane mode + wifi) so you'll also need to be > careful about the order of these if statements. in the airplane mode + wifi > example, wifi should take precedence. Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:1092: "invalid 'connection_type'"); On 2016/06/14 17:46:00, samuong wrote: > style nitpick: join the two lines above Done. https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.h (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.h:312: Status ExecuteSetEmulatedNetworkConditions(Session* session, On 2016/06/14 17:46:00, samuong wrote: > this is a really long name, let's change this to ExecuteSetNetworkConnection, > and be consistent with the mobile spec > > this change might need to be made in a few more places which have similarly > named functions (e.g. in command_executor.py) Changed this throughout the files where it is called/declared.
prasadv@chromium.org changed reviewers: + prasadv@chromium.org
https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:422: def testNetworkConditionsDifferentWebViews(self): Is the function unittest for ExecuteCommand? Generally we write unittests in seperate test file. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/server/http_handler.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/server/http_handler.cc:388: base::Bind(&ExecuteSetNetworkConnection))), Nit: Please correct indentation, also use only spaces, and indent 2 spaces at a time. CommandMapping( kPost, "session/:sessionId/network_connection", WrapToCommand("SetNetworkConnection", base::Bind(&ExecuteSetNetworkConnection))), https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:525: "invalid network_connections is missing 'connection_type'"); Error message is confusing. Do we have anything as invalid network_connections? or May be i'm missing something here? https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:538: network_conditions->upload_throughput = 30720 * 1024; I prefer using constants instant of using hard coded values. May be this is OK with ChromeDriver. I would ask Sam about this. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:585: //return web_view->OverrideNetworkConditions( Remove these lines if not needed.
https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/network_list.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/network_list.cc:25: "\"latency\": 0}]"; On 2016/06/14 17:45:59, samuong wrote: > nitpick: since this isn't a real change, let's remove this from the cl - let me > know if you need help doing this in git btw, after calling git-checkout, you might still need to add and commit the change... https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:414: style nit: delete this blank line https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:419: } style nit: you could fit these 5 lines on a single line https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:422: def testNetworkConditionsDifferentWebViews(self): On 2016/06/17 18:43:35, prasadv wrote: > Is the function unittest for ExecuteCommand? Generally we write unittests in > seperate test file. Yeah this shouldn't be here. Does anything even call it? https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/server/http_handler.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/server/http_handler.cc:388: base::Bind(&ExecuteSetNetworkConnection))), On 2016/06/17 18:43:35, prasadv wrote: > Nit: Please correct indentation, also use only spaces, and indent 2 spaces at a > time. > > CommandMapping( > kPost, > "session/:sessionId/network_connection", > WrapToCommand("SetNetworkConnection", > base::Bind(&ExecuteSetNetworkConnection))), > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head You can run "git cl presubmit" which will automatically check some of these style issues, including the one that Prasad mentioned. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:515: const int k2GMask = 0x20; You should put these at the top of the file in an anonymous namespace, so that they can be shared with Eva's function. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:538: network_conditions->upload_throughput = 30720 * 1024; On 2016/06/17 18:43:35, prasadv wrote: > I prefer using constants instant of using hard coded values. > May be this is OK with ChromeDriver. I would ask Sam about this. Agreed. Roisin can you add them and coordinate with Eva so that both functions use these constants?
https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/network_list.cc (right): https://codereview.chromium.org/2065733002/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/network_list.cc:25: "\"latency\": 0}]"; On 2016/06/17 22:01:30, samuong wrote: > On 2016/06/14 17:45:59, samuong wrote: > > nitpick: since this isn't a real change, let's remove this from the cl - let > me > > know if you need help doing this in git > > btw, after calling git-checkout, you might still need to add and commit the > change... Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:414: On 2016/06/17 22:01:30, samuong wrote: > style nit: delete this blank line Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:419: } On 2016/06/17 22:01:30, samuong wrote: > style nit: you could fit these 5 lines on a single line Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:422: def testNetworkConditionsDifferentWebViews(self): On 2016/06/17 22:01:30, samuong wrote: > On 2016/06/17 18:43:35, prasadv wrote: > > Is the function unittest for ExecuteCommand? Generally we write unittests in > > seperate test file. > > Yeah this shouldn't be here. Does anything even call it? It isn't called anywhere, I was unclear on the functionality when I wrote it. I deleted it. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/server/http_handler.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/server/http_handler.cc:388: base::Bind(&ExecuteSetNetworkConnection))), On 2016/06/17 22:01:30, samuong wrote: > On 2016/06/17 18:43:35, prasadv wrote: > > Nit: Please correct indentation, also use only spaces, and indent 2 spaces at > a > > time. > > > > CommandMapping( > > kPost, > > "session/:sessionId/network_connection", > > WrapToCommand("SetNetworkConnection", > > base::Bind(&ExecuteSetNetworkConnection))), > > > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head > > You can run "git cl presubmit" which will automatically check some of these > style issues, including the one that Prasad mentioned. Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:515: const int k2GMask = 0x20; On 2016/06/17 22:01:30, samuong wrote: > You should put these at the top of the file in an anonymous namespace, so that > they can be shared with Eva's function. Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:525: "invalid network_connections is missing 'connection_type'"); On 2016/06/17 18:43:35, prasadv wrote: > Error message is confusing. > Do we have anything as invalid network_connections? > or May be i'm missing something here? I changed this error message to be more clear. I changed the message to "invalid connection type". https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:538: network_conditions->upload_throughput = 30720 * 1024; On 2016/06/17 22:01:30, samuong wrote: > On 2016/06/17 18:43:35, prasadv wrote: > > I prefer using constants instant of using hard coded values. > > May be this is OK with ChromeDriver. I would ask Sam about this. > > Agreed. Roisin can you add them and coordinate with Eva so that both functions > use these constants? Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:538: network_conditions->upload_throughput = 30720 * 1024; On 2016/06/17 18:43:35, prasadv wrote: > I prefer using constants instant of using hard coded values. > May be this is OK with ChromeDriver. I would ask Sam about this. Done. https://codereview.chromium.org/2065733002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:585: //return web_view->OverrideNetworkConditions( On 2016/06/17 18:43:35, prasadv wrote: > Remove these lines if not needed. Done.
https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:413: def SetNetworkConnection(self, connection_type): Question: What is the difference between SetNetworkConditions and SetNetworkConnection, to me it looks like both these methods are doing same. Please correct me if I am missing anything here. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:887: Remove this blank line. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:888: # Network conditions must be set before it can be retrieved. Sam@ I'm not sure chromedriver unittesting guidelines but, looks like this test method is testing three difference conditions, preferably unit test should test single condition. This test method can be split into 3 tests. Same for other tests below https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:922: # Open a window with two divs couting successful + unsuccessful s/couting/counting https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:923: # attempts to complete XML task s/attemps/Attempts https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:924: self._driver.Load(self.GetHttpUrlForFile( Alternatively you can do this: self._driver.Load( self.GetHttpUrlForFile('/chromedriver/xmlrequest_test.html')) https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:942: # Open another window As per the style guide, you don't want to write comments for obvious statements, avoid describing the code. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:988: self.assertRaises(chromedriver.NoSuchElement, self.assertRaises( chromedriver.NoSuchElement, self._driver.FindElement, 'id', 'link') or self.assertRaises(chromedriver.NoSuchElement, self._driver.FindElement, 'id', 'link')
samuong@chromium.org changed reviewers: - samuong@google.com
thanks roisin, we're almost there, mostly style nitpicks now https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:413: def SetNetworkConnection(self, connection_type): On 2016/06/20 20:29:09, prasadv wrote: > Question: What is the difference between SetNetworkConditions and > SetNetworkConnection, to me it looks like both these methods are doing same. > Please correct me if I am missing anything here. These are different APIs to the same functionality. We're going to deprecate and remove the old one (SetNetworkConditions) because we've had trouble upstreaming the client-side changes due to interoperability concerns. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:41: const int kAirplaneMask = 0x1; nit: lets call it kAirplaneModeMask (same with kAirplaneLatency/Throughput as well) https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:532: nit: delete blank line https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:533: const base::DictionaryValue* parameters = NULL; use nullptr instead of NULL https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:536: nit: delete blank line https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:537: if(!parameters->GetInteger("type", &connection_type)) { nit: space after 'if' https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:539: "invalid 'connection_type'"); nit: join these two lines into one, remove braces https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:572: } if |connection_type| is 011 (airplane mode + wifi) then we want to enable the wifi, but with the way these if conditions are ordered, we'll turn everything offline. can we reorder this to behave more like what an android phone does? we should also be testing this condition. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:578: // iterate through and override each one's network conditions use full sentences for comments, and try to explain why you're doing things the way you are rather than what you're doing. in this case, you should mention that you need to apply the network conditions to every web view at the same time to ensure that network emulation is applied on a per-session basis. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:582: std::string web_view_id; nit: since this is only used within the for loop, you can declare it inside the for loop body to minimize the scope of the variable https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:888: # Network conditions must be set before it can be retrieved. On 2016/06/20 20:29:09, prasadv wrote: > Sam@ I'm not sure chromedriver unittesting guidelines but, looks like this test > method is testing three difference conditions, preferably unit test should test > single condition. This test method can be split into 3 tests. > Same for other tests below I think that makes sense - we could probably split this into testAirplaneModeEmulation, testWifiEmulation, etc. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/data/chrome... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/data/chrome... chrome/test/data/chromedriver/xmlrequest_test.html:86: </html> this file should be formatted according to https://google.github.io/styleguide/htmlcssguide.xml
[chromedriver] Added network connection tests and fixed WebView loop errors. Added individual tests for checking Wifi connection, Airplane Mode, and when connection_type's have more than one bit on. Added individual tests for 4G, 3G, and 2G connection in testEmulateNetworkConnection. Made changes to the loop in ExecuteSetNetworkConditions() to fix errors that were occurring. BUG=chromedriver:984 https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:41: const int kAirplaneMask = 0x1; On 2016/06/20 21:43:23, samuong wrote: > nit: lets call it kAirplaneModeMask (same with kAirplaneLatency/Throughput as > well) Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:532: On 2016/06/20 21:43:23, samuong wrote: > nit: delete blank line Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:533: const base::DictionaryValue* parameters = NULL; On 2016/06/20 21:43:23, samuong wrote: > use nullptr instead of NULL Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:536: On 2016/06/20 21:43:23, samuong wrote: > nit: delete blank line Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:537: if(!parameters->GetInteger("type", &connection_type)) { On 2016/06/20 21:43:23, samuong wrote: > nit: space after 'if' Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:539: "invalid 'connection_type'"); On 2016/06/20 21:43:23, samuong wrote: > nit: join these two lines into one, remove braces Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:572: } On 2016/06/20 21:43:23, samuong wrote: > if |connection_type| is 011 (airplane mode + wifi) then we want to enable the > wifi, but with the way these if conditions are ordered, we'll turn everything > offline. can we reorder this to behave more like what an android phone does? we > should also be testing this condition. I changed the orders to set the connection to the highest selected bit. I also added tests for when more than one bit is 1. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:578: // iterate through and override each one's network conditions On 2016/06/20 21:43:23, samuong wrote: > use full sentences for comments, and try to explain why you're doing things the > way you are rather than what you're doing. in this case, you should mention that > you need to apply the network conditions to every web view at the same time to > ensure that network emulation is applied on a per-session basis. Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:582: std::string web_view_id; On 2016/06/20 21:43:23, samuong wrote: > nit: since this is only used within the for loop, you can declare it inside the > for loop body to minimize the scope of the variable Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:887: On 2016/06/20 20:29:09, prasadv wrote: > Remove this blank line. Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:888: # Network conditions must be set before it can be retrieved. On 2016/06/20 21:43:23, samuong wrote: > On 2016/06/20 20:29:09, prasadv wrote: > > Sam@ I'm not sure chromedriver unittesting guidelines but, looks like this > test > > method is testing three difference conditions, preferably unit test should > test > > single condition. This test method can be split into 3 tests. > > Same for other tests below > > I think that makes sense - we could probably split this into > testAirplaneModeEmulation, testWifiEmulation, etc. I added three tests dealing with the different types of connections, and one testing that the correct network is chosen when multiple bits are 1. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:922: # Open a window with two divs couting successful + unsuccessful On 2016/06/20 20:29:09, prasadv wrote: > s/couting/counting Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:923: # attempts to complete XML task On 2016/06/20 20:29:09, prasadv wrote: > s/attemps/Attempts This is meant to read as one sentence broken up for line length. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:924: self._driver.Load(self.GetHttpUrlForFile( On 2016/06/20 20:29:09, prasadv wrote: > Alternatively you can do this: > > self._driver.Load( > self.GetHttpUrlForFile('/chromedriver/xmlrequest_test.html')) Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:942: # Open another window On 2016/06/20 20:29:10, prasadv wrote: > As per the style guide, you don't want to write comments for obvious statements, > avoid describing the code. Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:988: self.assertRaises(chromedriver.NoSuchElement, On 2016/06/20 20:29:10, prasadv wrote: > self.assertRaises( > chromedriver.NoSuchElement, self._driver.FindElement, 'id', 'link') > > or > > self.assertRaises(chromedriver.NoSuchElement, > self._driver.FindElement, > 'id', > 'link') Done. https://codereview.chromium.org/2065733002/diff/40001/chrome/test/data/chrome... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/40001/chrome/test/data/chrome... chrome/test/data/chromedriver/xmlrequest_test.html:86: </html> On 2016/06/20 21:43:23, samuong wrote: > this file should be formatted according to > https://google.github.io/styleguide/htmlcssguide.xml Done.
Description was changed from ========== Added a method to set the overridden network conditionsin the session. Added a test for the method, and another test to look at network conditions between different windows. BUG= ========== to ========== [chromedriver] Add a method to override the network conditions of the ChromeDriver session. Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline. BUG=chromedriver:984 ==========
https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:211: } nit: remove the space before the } https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.h (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.h:60: int GetNetworkConnection(); you can make this a const function https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:42: nit: delete these two blank lines https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:61: const bool kOffline = true; i don't think these are necessary, saying "offline = true" or "offline = false" fine https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:536: params.GetDictionary("parameters", ¶meters); the return value for this call is not checked, and if it fails we won't notice and the next line will segfault you can actually use GetInteger("parameters.type", &connection_type) instead, which avoids the need for two separate calls https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:543: if (!desktop) { for consistency, use "if (status.IsError())" also a nit: this if statement only has one line, so we don't need braces https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:574: network_conditions->download_throughput = kAirplaneModeThroughput; do we actually need to set latency and throughput when we've already set offline=true? https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:577: return Status(kUnknownError, "invalid 'connection_type'"); should this be an error? if i've got airplane mode off, and also have wifi and data switched off, my phone won't be connected. maybe just set it to offline in this case? https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:587: session->chrome->GetWebViewIds(&web_view_ids); don't forget to check the error status https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:593: ++web_view_itr) { you can use a range-based for loop here for an example, see http://en.cppreference.com/w/cpp/language/range-for https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:599: } nit: remove braces for this, and any other single-line blocks
https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:211: } On 2016/06/27 21:31:53, samuong wrote: > nit: remove the space before the } Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.h (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.h:60: int GetNetworkConnection(); On 2016/06/27 21:31:53, samuong wrote: > you can make this a const function Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:42: On 2016/06/27 21:31:53, samuong wrote: > nit: delete these two blank lines Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:61: const bool kOffline = true; On 2016/06/27 21:31:53, samuong wrote: > i don't think these are necessary, saying "offline = true" or "offline = false" > fine Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:543: if (!desktop) { On 2016/06/27 21:31:53, samuong wrote: > for consistency, use "if (status.IsError())" > also a nit: this if statement only has one line, so we don't need braces Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:574: network_conditions->download_throughput = kAirplaneModeThroughput; On 2016/06/27 21:31:53, samuong wrote: > do we actually need to set latency and throughput when we've already set > offline=true? I left these being set to zero in case the latency or throughput was ever accessed in another function. However if these aren't being used when offline=true then I can get rid of them. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:577: return Status(kUnknownError, "invalid 'connection_type'"); On 2016/06/27 21:31:53, samuong wrote: > should this be an error? if i've got airplane mode off, and also have wifi and > data switched off, my phone won't be connected. maybe just set it to offline in > this case? Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:587: session->chrome->GetWebViewIds(&web_view_ids); On 2016/06/27 21:31:53, samuong wrote: > don't forget to check the error status Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:593: ++web_view_itr) { On 2016/06/27 21:31:53, samuong wrote: > you can use a range-based for loop here > for an example, see http://en.cppreference.com/w/cpp/language/range-for Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:599: } On 2016/06/27 21:31:53, samuong wrote: > nit: remove braces for this, and any other single-line blocks Done.
https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:574: network_conditions->download_throughput = kAirplaneModeThroughput; On 2016/06/28 18:36:48, roisinmcl wrote: > On 2016/06/27 21:31:53, samuong wrote: > > do we actually need to set latency and throughput when we've already set > > offline=true? > > I left these being set to zero in case the latency or throughput was ever > accessed in another function. However if these aren't being used when > offline=true then I can get rid of them. It's probably fine either way. Since this if branch, and the else branch below it, are both doing the same thing, we should probably combine them. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:205: int ChromeDesktopImpl::GetNetworkConnectionValue() const { nit: remove "Value" suffix If you feel "GetNetworkConnection" sounds awkward, how about we rename it to "connection_type_" and "GetConnectionType"? https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:209: void ChromeDesktopImpl::SetNetworkConnectionValue( nit: remove "Value" suffix https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/client/command_executor.py (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/client/command_executor.py:149: GET_EMULATED_NETWORK_CONDITIONS = ( nit: should probably call this "GET_NETWORK_CONNECTION" for consistency https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1463: print 'something else happenend' we forgot to delete this while we were debugging, please remove it before landing https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:1: <html> add a <!DOCTYPE html> at the top- https://google.github.io/styleguide/htmlcssguide.xml#Document_Type https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:6: <span id='requestButton' style="cursor: pointer; text-decoration: underline"> is the style attribute necessary here? https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:6: <span id='requestButton' style="cursor: pointer; text-decoration: underline"> keep indents to 2 spaces - https://google.github.io/styleguide/htmlcssguide.xml#Indentation https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:54: if (httpRequest.status == 200 ) { nit: no space before the )s https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:58: console.log(httpRequest.status) are we using the console log or the divs to report status? https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:71: setInterval(makeRequest,300); nit: space after the ','
https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:536: params.GetDictionary("parameters", ¶meters); On 2016/06/27 21:31:53, samuong wrote: > the return value for this call is not checked, and if it fails we won't notice > and the next line will segfault > > you can actually use GetInteger("parameters.type", &connection_type) instead, > which avoids the need for two separate calls Done. https://codereview.chromium.org/2065733002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/session_commands.cc:574: network_conditions->download_throughput = kAirplaneModeThroughput; On 2016/06/28 23:23:40, samuong wrote: > On 2016/06/28 18:36:48, roisinmcl wrote: > > On 2016/06/27 21:31:53, samuong wrote: > > > do we actually need to set latency and throughput when we've already set > > > offline=true? > > > > I left these being set to zero in case the latency or throughput was ever > > accessed in another function. However if these aren't being used when > > offline=true then I can get rid of them. > > It's probably fine either way. Since this if branch, and the else branch below > it, are both doing the same thing, we should probably combine them. Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:205: int ChromeDesktopImpl::GetNetworkConnectionValue() const { On 2016/06/28 23:23:40, samuong wrote: > nit: remove "Value" suffix > > If you feel "GetNetworkConnection" sounds awkward, how about we rename it to > "connection_type_" and "GetConnectionType"? Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:209: void ChromeDesktopImpl::SetNetworkConnectionValue( On 2016/06/28 23:23:40, samuong wrote: > nit: remove "Value" suffix Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/client/command_executor.py (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/client/command_executor.py:149: GET_EMULATED_NETWORK_CONDITIONS = ( On 2016/06/28 23:23:40, samuong wrote: > nit: should probably call this "GET_NETWORK_CONNECTION" for consistency This was mostly a placeholder for Eva's code, so I deleted this as it is not needed for my setter method's functionality or testing. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1463: print 'something else happenend' On 2016/06/28 23:23:40, samuong wrote: > we forgot to delete this while we were debugging, please remove it before > landing Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:6: <span id='requestButton' style="cursor: pointer; text-decoration: underline"> On 2016/06/28 23:23:40, samuong wrote: > is the style attribute necessary here? Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:54: if (httpRequest.status == 200 ) { On 2016/06/28 23:23:41, samuong wrote: > nit: no space before the )s Done. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:58: console.log(httpRequest.status) On 2016/06/28 23:23:40, samuong wrote: > are we using the console log or the divs to report status? We should be using the divs, so I deleted the console logs, which were just repeating what was in the divs. https://codereview.chromium.org/2065733002/diff/100001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:71: setInterval(makeRequest,300); On 2016/06/28 23:23:41, samuong wrote: > nit: space after the ',' Done.
almost there, just a few more comments https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... File chrome/test/chromedriver/chrome/chrome_desktop_impl.h (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.h:61: void SetNetworkConnection(const int& network_connection); "const int&" seems needlessly complex, is there any reason to not just use "int"? https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:898: self.assertEquals(network['offline'], False) this looks ok for now but after eva submits her cl we should go back and convert this to use her getter command https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:962: def testNetworkConnectionAcrossTabs(self): change name to: testNetworkConnectionTypeIsAppliedToAllTabsImmediately https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:968: self.assertEquals(network['upload_throughput'], 750 * 1024) i think it's unnecessary to check latency and throughput here (and in other parts of this test). what's important is whether we're online (or offline), so that assertion should suffice https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:976: time.sleep(10) do we actually need to have this call to sleep? generally we prefer not to call sleep in test code, since it leads to flaky (non-deterministic) test failures. maybe wait until the number of xhrs reaches a certain value? https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:985: '/helloworld', respondWithString) the xmlrequest_test.html page makes requests to this endpoint, don't you need to set it up before loading the page? https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:988: 1, self._driver.ExecuteScript('window.name = "oldWindow"; return 1;')) is it necessary to set window.name here? i don't see this getting used later in this test... https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1014: def testNetworkConditionsDifferentWebViews(self): change the name of this test to: testNetworkConnectionTypeIsAppliedToAllTabs https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1021: 1, self._driver.ExecuteScript('window.name = "oldWindow"; return 1;')) is it necessary to set window.name here? it's used later, but you could just use "self._driver.SwitchToWindow(window1_handle)" instead (as you've done in the test above) https://codereview.chromium.org/2065733002/diff/120001/chrome/test/data/chrom... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:68: </body> the indenting for this file still looks incorrect. you might need to set up sublime to expand all tabs to 2 spaces? https://chromium.googlesource.com/chromium/src/+/master/docs/linux_sublime_de...
https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... File chrome/test/chromedriver/chrome/chrome_desktop_impl.h (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/chrome/chrome_desktop_impl.h:61: void SetNetworkConnection(const int& network_connection); On 2016/06/29 18:37:54, samuong wrote: > "const int&" seems needlessly complex, is there any reason to not just use > "int"? Done. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:962: def testNetworkConnectionAcrossTabs(self): On 2016/06/29 18:37:54, samuong wrote: > change name to: testNetworkConnectionTypeIsAppliedToAllTabsImmediately Done. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:968: self.assertEquals(network['upload_throughput'], 750 * 1024) On 2016/06/29 18:37:54, samuong wrote: > i think it's unnecessary to check latency and throughput here (and in other > parts of this test). what's important is whether we're online (or offline), so > that assertion should suffice Done. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:976: time.sleep(10) On 2016/06/29 18:37:54, samuong wrote: > do we actually need to have this call to sleep? > > generally we prefer not to call sleep in test code, since it leads to flaky > (non-deterministic) test failures. maybe wait until the number of xhrs reaches a > certain value? I was using this to make sure the tests ran correctly when I was writing them, and I forgot to delete this. It is deleted now. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:985: '/helloworld', respondWithString) On 2016/06/29 18:37:54, samuong wrote: > the xmlrequest_test.html page makes requests to this endpoint, don't you need to > set it up before loading the page? xmlrequest_test.html doesn't use this endpoint until it makes a request by clicking the "requestbutton", which is after this endpoint is defined. I think it should work the same way, but I moved it above loading the page for better organization. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:988: 1, self._driver.ExecuteScript('window.name = "oldWindow"; return 1;')) On 2016/06/29 18:37:54, samuong wrote: > is it necessary to set window.name here? i don't see this getting used later in > this test... I removed this line. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1014: def testNetworkConditionsDifferentWebViews(self): On 2016/06/29 18:37:54, samuong wrote: > change the name of this test to: testNetworkConnectionTypeIsAppliedToAllTabs Done. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:1021: 1, self._driver.ExecuteScript('window.name = "oldWindow"; return 1;')) On 2016/06/29 18:37:54, samuong wrote: > is it necessary to set window.name here? it's used later, but you could just use > "self._driver.SwitchToWindow(window1_handle)" instead (as you've done in the > test above) Done. https://codereview.chromium.org/2065733002/diff/120001/chrome/test/data/chrom... File chrome/test/data/chromedriver/xmlrequest_test.html (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/data/chrom... chrome/test/data/chromedriver/xmlrequest_test.html:68: </body> On 2016/06/29 18:37:55, samuong wrote: > the indenting for this file still looks incorrect. you might need to set up > sublime to expand all tabs to 2 spaces? > > https://chromium.googlesource.com/chromium/src/+/master/docs/linux_sublime_de... Done.
https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/2065733002/diff/120001/chrome/test/chromedriv... chrome/test/chromedriver/test/run_py_tests.py:985: '/helloworld', respondWithString) On 2016/06/29 19:45:00, roisinmcl wrote: > On 2016/06/29 18:37:54, samuong wrote: > > the xmlrequest_test.html page makes requests to this endpoint, don't you need > to > > set it up before loading the page? > > xmlrequest_test.html doesn't use this endpoint until it makes a request by > clicking the "requestbutton", which is after this endpoint is defined. I think > it should work the same way, but I moved it above loading the page for better > organization. for consistency, can you move it to the top of the function? (before setting the network to online) i think other than this, we're good to go
lgtm
The CQ bit was checked by roisinmcl@google.com
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 ========== [chromedriver] Add a method to override the network conditions of the ChromeDriver session. Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline. BUG=chromedriver:984 ========== to ========== [chromedriver] Add a method to override the network conditions of the ChromeDriver session. Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline. BUG=chromedriver:984 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [chromedriver] Add a method to override the network conditions of the ChromeDriver session. Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline. BUG=chromedriver:984 ========== to ========== [chromedriver] Add a method to override the network conditions of the ChromeDriver session. Allows users to set the overridden network conditions of the session to preset values including 2G, 3G, 4G, Wifi, and Offline. BUG=chromedriver:984 Committed: https://crrev.com/17a2e1eae574d917f9ccb1eeccf8f11426ac5588 Cr-Commit-Position: refs/heads/master@{#403209} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/17a2e1eae574d917f9ccb1eeccf8f11426ac5588 Cr-Commit-Position: refs/heads/master@{#403209} |
