Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6340008:
Adding instant tests for popup handling and search query length 100 character... (Closed)
|
Created:
9 years, 11 months ago by Huyen Modified:
9 years, 6 months ago CC:
chromium-reviews, John Grabowski, anantha, dyu1, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdding instant tests for popup handling and search query length 100 characters limit.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72045
Patch Set 1 #
Total comments: 28
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #
Total comments: 5
Patch Set 4 : '' #Messages
Total messages: 13 (0 generated)
Hey guys, I'm adding a few tests for instant's behavior when loading website with popups and search query longer than 100 characters. Please take a look. Thanks, Huyen
http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:74: popups_site = ('http://www.javascript-coder.com/files/' does instant work with file:// urls? if so, should probably use that for this kind of test case (or some not-yet-implemented test server we control. app engine maybe?) http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:85: """Test that instant loads for search querty of 100 characters.""" this test case has no failure condition http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:93: ahundredone = ('##################################################' probably not the best variable name. maybe something more readable like long_string and longer_string http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:97: time.sleep(2) need an alternative to sleep. not reliable/efficient. also, from what we saw before, I don't think this test would work. I would expect self.assertTrue(self._NotLoading()) to pass regardless of whether or not the query was sent to instant.
http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:26: """Sometime instant will decide not to prefetch a query.""" this docstring is not clear http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:74: popups_site = ('http://www.javascript-coder.com/files/' On 2011/01/19 00:31:32, Allen wrote: > does instant work with file:// urls? if so, should probably use that for this > kind of test case (or some not-yet-implemented test server we control. app > engine maybe?) Use one of the files used in popups.py http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:82: self.assertEqual(0, len(blocked_popups), msg='Popup not disabled.') correct the msg string to apply better here. something like 'Did not expect popups during instant preview' http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:84: def testInstantSearchQueryLimit_100Chars(self): no _ in fn name please http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:86: ahundred = ('##################################################' a_hundred = '#' * 100 http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:91: def testInstantSearchQueryLimit_101Chars(self): can be merged with the previous test. and renamed: testInstantSearchQueryLimits http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:92: """Test that instant does not load for search querty of 101 characters.""" s/querty/query/ http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:97: time.sleep(2) +1 do not use sleep(). You're asking for trouble. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:99: leave 2 blank lines at global scope
http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:85: """Test that instant loads for search querty of 100 characters.""" On 2011/01/19 00:31:32, Allen wrote: > this test case has no failure condition Just to be clear, this test is basically fine, it's just that the test "done loading" test is really just "not loading", which doesn't necessarily imply that anything was loaded. Another line checking the location would just make it feel more complete.
http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:25: def _NotLoading(self): Is this needed? If instant is not active you can have assertFalse on the "active" dictionary value. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:74: popups_site = ('http://www.javascript-coder.com/files/' Can use something like popup_url = self.GetFileURLForPath(os.path.join(self.DataDir(), 'javascript-popup-example3.html')) http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:97: time.sleep(2) You can use WaitUntil and check for a specific DOM on the page. Maybe something with the attr_dict.
http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:97: time.sleep(2) On 2011/01/19 23:14:45, dyu1 wrote: > You can use WaitUntil and check for a specific DOM on the page. Maybe something > with the attr_dict. in this case seeing a dom is not expected, but huyen is looking into alternatives for detecting the "press enter to search" ui
Everyone, I've addressed all the comments. I've added _DoneLoadingQuery() to check that a "q=" param is in the location URL. So the test wait until a particular query string is present in the location to verify that Instant did submit a search. I've removed the test case where the query string is 100+ b/c I'm still working on a more reliable way to do this. Thanks! H http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.py File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:74: popups_site = ('http://www.javascript-coder.com/files/' On 2011/01/19 00:31:32, Allen wrote: > does instant work with file:// urls? if so, should probably use that for this > kind of test case (or some not-yet-implemented test server we control. app > engine maybe?) Looks like it does. I've changed the test to use an existing popup HTML page in the data dir. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:82: self.assertEqual(0, len(blocked_popups), msg='Popup not disabled.') On 2011/01/19 02:04:36, Nirnimesh wrote: > correct the msg string to apply better here. something like > 'Did not expect popups during instant preview' Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:84: def testInstantSearchQueryLimit_100Chars(self): On 2011/01/19 02:04:36, Nirnimesh wrote: > no _ in fn name please Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:85: """Test that instant loads for search querty of 100 characters.""" On 2011/01/19 00:31:32, Allen wrote: > this test case has no failure condition Fixed by waiting until the query string shows up in the location URL. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:86: ahundred = ('##################################################' On 2011/01/19 02:04:36, Nirnimesh wrote: > a_hundred = '#' * 100 Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:91: def testInstantSearchQueryLimit_101Chars(self): On 2011/01/19 02:04:36, Nirnimesh wrote: > can be merged with the previous test. > > and renamed: testInstantSearchQueryLimits Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:92: """Test that instant does not load for search querty of 101 characters.""" On 2011/01/19 02:04:36, Nirnimesh wrote: > s/querty/query/ Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:93: ahundredone = ('##################################################' On 2011/01/19 00:31:32, Allen wrote: > probably not the best variable name. maybe something more readable like > long_string and longer_string Done. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:97: time.sleep(2) On 2011/01/19 23:14:45, dyu1 wrote: > You can use WaitUntil and check for a specific DOM on the page. Maybe something > with the attr_dict. I'm taking this test case out of this CL for now. Talking with developer to find a better way to test this scenario. http://codereview.chromium.org/6340008/diff/1/chrome/test/functional/instant.... chrome/test/functional/instant.py:99: On 2011/01/19 02:04:36, Nirnimesh wrote: > leave 2 blank lines at global scope Done.
http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... chrome/test/functional/instant.py:25: def _DoneLoadingQuery(self, query): Add a docstring to explain what it's doing. http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... chrome/test/functional/instant.py:30: else: else: is redundant
http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... chrome/test/functional/instant.py:25: def _DoneLoadingQuery(self, query): On 2011/01/20 00:39:23, Nirnimesh wrote: > Add a docstring to explain what it's doing. Done. http://codereview.chromium.org/6340008/diff/9001/chrome/test/functional/insta... chrome/test/functional/instant.py:30: else: On 2011/01/20 00:39:23, Nirnimesh wrote: > else: is redundant Polished the logic here a bit. Should be better now. :)
http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... chrome/test/functional/instant.py:30: query: Value of query parameter. So this makes it very specific to google search. Rename the function to make this explicit (add Google in the name) http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... chrome/test/functional/instant.py:31: E.g., http://www.google.com?q=hi so query is 'hi'. So this makes it specific to google search URLs. Rename function to make this explicit (add 'Google' in it) http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... chrome/test/functional/instant.py:32: """ Allen, all these DoneLoading, and DoneLoadingQuery "hacks" are getting nasty. We need a better solution here. Maybe chat with sky@ about it.
http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... File chrome/test/functional/instant.py (right): http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... chrome/test/functional/instant.py:30: query: Value of query parameter. On 2011/01/20 18:53:32, Nirnimesh wrote: > So this makes it very specific to google search. > Rename the function to make this explicit (add Google in the name) Done. http://codereview.chromium.org/6340008/diff/14001/chrome/test/functional/inst... chrome/test/functional/instant.py:31: E.g., http://www.google.com?q=hi so query is 'hi'. On 2011/01/20 18:53:32, Nirnimesh wrote: > So this makes it specific to google search URLs. > Rename function to make this explicit (add 'Google' in it) Done.
LGTM Make sure you run the test several times (use --repeat=20) before committing.
Committed |
