Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5115007:
Omnibox tests for,... (Closed)
DescriptionOmnibox tests for recent pages and content history
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70002
Patch Set 1 #Patch Set 2 : '' #
Total comments: 2
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 5
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 3
Patch Set 7 : '' #
Total comments: 2
Patch Set 8 : '' #Messages
Total messages: 12 (0 generated)
Please update the title of this CL to be a one-line summary. http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:227: time.sleep(1) no sleep please. use WaitUntil http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py#newcod... functional/omnibox.py:240: time.sleep(40) no sleep please. Use waitUntil so that you end up waiting only as much as needed, not more. is 40 secs a known value? Is it the same on all platforms?
ping?
I have uploaded the tests with changes. Thanks, Rohit On 2010/12/11 01:06:51, Nirnimesh wrote: > ping?
On 2010/11/22 19:48:14, Nirnimesh wrote: > Please update the title of this CL to be a one-line summary. > > http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:227: time.sleep(1) > no sleep please. use WaitUntil Done. > > http://codereview.chromium.org/5115007/diff/2001/functional/omnibox.py#newcod... > functional/omnibox.py:240: time.sleep(40) > no sleep please. Use waitUntil so that you end up waiting only as much as > needed, not more. > is 40 secs a known value? Is it the same on all platforms? Done. Yes, it is the same on Windows as well.
http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... functional/omnibox.py:217: remove stray whitespace chars http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... functional/omnibox.py:236: def _CheckMatches(self, old_matches_len, search_text): Be more descriptive with the name. Or at least provide a descriptive docstring http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... functional/omnibox.py:253: self.WaitUntil(lambda: self._CheckMatches(len(old_matches), search_text)) wrap this inside self.assertTrue() Be aware that waitUntil has a timeout of ~30 secs http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... functional/omnibox.py:258: def testRecentPageHistory(self): In the interest of saving time it might be useful to somehow combine this with the last test. http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... functional/omnibox.py:268: self.WaitUntil(lambda: self._CheckMatches(len(old_matches), search_text), wrap inside self.assertTrue
Diff delta may not look good. Not sure what went wrong with uploading code. I did sync code and then diff got messed up. svn diff omnibox.py always look good. I uploaded the changes. Thanks, Rohit On 2010/12/22 19:32:51, Nirnimesh wrote: > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... > functional/omnibox.py:217: > remove stray whitespace chars Done. It was code uploading problem. > > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... > functional/omnibox.py:236: def _CheckMatches(self, old_matches_len, > search_text): > Be more descriptive with the name. > Or at least provide a descriptive docstring Done. > > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... > functional/omnibox.py:253: self.WaitUntil(lambda: > self._CheckMatches(len(old_matches), search_text)) > wrap this inside self.assertTrue() Done. > > Be aware that waitUntil has a timeout of ~30 secs Set max timeout of 1 sec. > > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... > functional/omnibox.py:258: def testRecentPageHistory(self): > In the interest of saving time it might be useful to somehow combine this with > the last test. Above test takes max 1 sec of delay. If we still want to combine this test with above one, let me know. > > http://codereview.chromium.org/5115007/diff/13001/functional/omnibox.py#newco... > functional/omnibox.py:268: self.WaitUntil(lambda: > self._CheckMatches(len(old_matches), search_text), > wrap inside self.assertTrue Done.
http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... functional/omnibox.py:253: I still see this stray space char http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... functional/omnibox.py:282: """Compares omnibox results new count with old one. If new count is greater Remove the 2nd line, the code explains that. Rename fn to _GotNewMatches() """Determines if omnibox has any new matches""" http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... functional/omnibox.py:316: self.assertTrue(self.WaitUntil(lambda: self._CheckMatches(len(old_matches), move lambda to next line, indented by 4 spaces only wrt this line
I have uploaded the code with changes. Thanks, Rohit On 2010/12/22 21:56:10, Nirnimesh wrote: > http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... > functional/omnibox.py:253: > I still see this stray space char Done. > > http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... > functional/omnibox.py:282: """Compares omnibox results new count with old one. > If new count is greater > Remove the 2nd line, the code explains that. > > Rename fn to _GotNewMatches() > """Determines if omnibox has any new matches""" Done. > > http://codereview.chromium.org/5115007/diff/22001/functional/omnibox.py#newco... > functional/omnibox.py:316: self.assertTrue(self.WaitUntil(lambda: > self._CheckMatches(len(old_matches), > move lambda to next line, indented by 4 spaces only wrt this line Done.
http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py File functional/omnibox.py (right): http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py#newco... functional/omnibox.py:299: self.assertTrue(self.WaitUntil(lambda: self._CheckMatches(len(old_matches), Update this http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py#newco... functional/omnibox.py:316: lambda: self._CheckMatches(len(old_matches), search_text), timeout=40)) and this
Oops...I have uploaded the code. Thanks, Rohit On 2010/12/22 22:58:17, Nirnimesh wrote: > http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py > File functional/omnibox.py (right): > > http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py#newco... > functional/omnibox.py:299: self.assertTrue(self.WaitUntil(lambda: > self._CheckMatches(len(old_matches), > Update this Done. > > http://codereview.chromium.org/5115007/diff/26001/functional/omnibox.py#newco... > functional/omnibox.py:316: lambda: self._CheckMatches(len(old_matches), > search_text), timeout=40)) > and this Done.
LGTM. Will commit |
