Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9420037:
reuse the same browser when running webdriver tests (Closed)
|
Created:
8 years, 10 months ago by Jennifer Messerly Modified:
8 years, 2 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionThis change runs WebDriver tests in batch mode. This means that it keeps N browsers open, where N is the number of jobs in test.dart. It then passes each in-browser test to the next available browser. This is analogous to how DartC tests reuse the java process.
It seems to cut about 30-40% off the run time of tests on Mac+Firefox, with roughly similar gains for other browsers. It also might reduce the browser startup flakiness--although this needs more testing.
I reused almost all of the logic and protocol from DartC's batch runner. Most of the changes are in run_selenium.py, which now implements the batch protocol when started with --batch.
A few changes to BatchProcessRunner:
1. it can now pass arguments when starting the batch process. We need this to start "python run_selenium.py --batch"
2. related to #1, it needs to remove the run_selenium argument when running each individual test.
3. added a graceful shutdown command so run_selenium can shutdown. I would've preferred SIGTERM or SIGINT, but it doesn't look like that's supported in the Process API (sends only SIGKILL).
Changes to run_selenium.py:
1. Add a batch mode
2. Factored the common functionality to preserve "single run" mode
Committed: https://code.google.com/p/dart/source/detail?r=4491
Patch Set 1 #Patch Set 2 : updated #
Total comments: 41
Patch Set 3 : updated #Patch Set 4 : comment fixes #Patch Set 5 : added comment about processQueue #
Total comments: 1
Messages
Total messages: 8 (0 generated)
On 2012/02/18 01:43:51, John Messerly wrote: btw -- no rush to get this change in, since our bots are in pretty good shape right now :) Cya all next week. Cheers, - John
https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:102: List<String> get batchRunnerArguments() => ['-batch']; Why use one dash here (-batch) and two dashes below (--batch)? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:260: RunningProcess(TestCase this.testCase, this.processQueue); Careful here. The VM uses the RunningProcess class in their own standalone test suite. Either we need to modify their standalone test framework with the updated constructor for RunningProcess, or make processQueue optional. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:417: new Timer((e) { if (!closed) _process.kill(); }, 30000); Where's 30000 coming from? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:606: 'safari': ['Safari'], 'ff': ['firefox', 'firefox-bin'], (+ firefox-bin) yay. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_suite.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_suite.dart:537: '--browser=${configuration["browser"]}', Not that it really matters, but why did we change the order these arguments are passed here? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... File tools/testing/run_selenium.py (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:9: the result. It uses Selenium WebDriver for running the tests. Selenium RC and Selenium Webdriver (Selenium Webdriver is used for everything but Safari, which uses plain-jane Selenium, also called Selenium Remote Control). https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:12: the same browser process. Batching gives faster throughput and makes tests less subject to browser starting flakiness, issues with too many browser processes running, etc. line > 80 char. Also maybe give an example how the two different cases are invoked. Explain how the browser is specified by reading from stdin in the batch case, for example. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:48: return run_test_in_browser_selenium1(browser, html_out, timeout, is_perf) I know I came up with the name originally here, but maybe call the method run_tests_in_browser_selenium_rc instead of selenium1. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:104: def start_browser(browser, html_out): Maybe call this get_browser instead of start_browser since we're returning the newly created browser runner? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:121: # !!!! Do we have a workaround for this yet? We probably shouldn't comment out the two lines below until we do. :-/ https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:144: print '!!! trying to close browser !!!' Do we always want to print this? Perhaps a more informative error message saying that a timeout exception has occurred? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:150: # TODO(efortuna): figure out why this crashes.... and avoid? nit: Yes, this was my mistake, but let's make that a capital "F" at the start of the sentence, per style guidelines. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:174: print unicode(source[index : end_index]).encode("utf-8") good catch here. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:177: def run_batch_tests(): Add some comments saying what's going on in this method. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:183: # SIGTERM instead. TODO(jmesserly): make this more robust FYI: I've filed a feature request about this here: https://code.google.com/p/dart/issues/detail?id=1756 Maybe add issue # in comment here? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:216: print '>>> TEST PASS' I believe you need to print out the following "magic string" if a test passed: 'Content-Type: text/plain\nPASS' (see line 164 for example).
PTAL https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:102: List<String> get batchRunnerArguments() => ['-batch']; On 2012/02/21 18:19:35, Emily Fortuna wrote: > Why use one dash here (-batch) and two dashes below (--batch)? I didn't want to change the DartC test runner. Likewise, I don't think Python's option parser library will handle a single "-". It's ugly but I'm not sure what else to do. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:260: RunningProcess(TestCase this.testCase, this.processQueue); On 2012/02/21 18:19:35, Emily Fortuna wrote: > Careful here. The VM uses the RunningProcess class in their own standalone test > suite. Either we need to modify their standalone test framework with the updated > constructor for RunningProcess, or make processQueue optional. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:417: new Timer((e) { if (!closed) _process.kill(); }, 30000); On 2012/02/21 18:19:35, Emily Fortuna wrote: > Where's 30000 coming from? Needed a timeout :) Unfortunately, we don't have access to a testCase at this point. And it's not clear if that would be the right timeout to use, since this is a timeout for how long we think run_selenium will take to shutdown, not how long an individual test needs. I thought about storing it in a variable, but I wasn't sure that a level of indirection adds anything. The hope was that the line was fairly self explanatory ("in 30s, if the process is not closed, kill it") https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_suite.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_suite.dart:537: '--browser=${configuration["browser"]}', On 2012/02/21 18:19:35, Emily Fortuna wrote: > Not that it really matters, but why did we change the order these arguments are > passed here? At one point I thought I needed --browser first so that could be configured at startup of run_selenium. The idea is the first 2 args would be part of the "batch mode" command line, and the last 2 args would be part of the "per test" command line. I ended up making it handle the --browser though, so this could be reverted. WDYT? https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... File tools/testing/run_selenium.py (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:9: the result. It uses Selenium WebDriver for running the tests. On 2012/02/21 18:19:35, Emily Fortuna wrote: > Selenium RC and Selenium Webdriver (Selenium Webdriver is used for everything > but Safari, which uses plain-jane Selenium, also called Selenium Remote > Control). Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:12: the same browser process. Batching gives faster throughput and makes tests less subject to browser starting flakiness, issues with too many browser processes running, etc. On 2012/02/21 18:19:35, Emily Fortuna wrote: > line > 80 char. > Also maybe give an example how the two different cases are invoked. Explain how > the browser is specified by reading from stdin in the batch case, for example. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:48: return run_test_in_browser_selenium1(browser, html_out, timeout, is_perf) On 2012/02/21 18:19:35, Emily Fortuna wrote: > I know I came up with the name originally here, but maybe call the method > run_tests_in_browser_selenium_rc instead of selenium1. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:104: def start_browser(browser, html_out): On 2012/02/21 18:19:35, Emily Fortuna wrote: > Maybe call this get_browser instead of start_browser since we're returning the > newly created browser runner? I worry about calling it "get" in that it sounds less side-effecty. It could be get_and_start_browser? But since most functions "get" something, I usually drop the "get" (might be because I'm used to APIs like this: http://msdn.microsoft.com/en-us/library/system.diagnostics.process.start.aspx) https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:121: # !!!! On 2012/02/21 18:19:35, Emily Fortuna wrote: > Do we have a workaround for this yet? We probably shouldn't comment out the two > lines below until we do. :-/ Oops! Thanks for catching that. Fixed--now checks if the file exists and copies it if it does. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:144: print '!!! trying to close browser !!!' On 2012/02/21 18:19:35, Emily Fortuna wrote: > Do we always want to print this? Perhaps a more informative error message saying > that a timeout exception has occurred? Oops. removed. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:150: # TODO(efortuna): figure out why this crashes.... and avoid? On 2012/02/21 18:19:35, Emily Fortuna wrote: > nit: Yes, this was my mistake, but let's make that a capital "F" at the start of > the sentence, per style guidelines. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:174: print unicode(source[index : end_index]).encode("utf-8") On 2012/02/21 18:19:35, Emily Fortuna wrote: > good catch here. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:177: def run_batch_tests(): On 2012/02/21 18:19:35, Emily Fortuna wrote: > Add some comments saying what's going on in this method. Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:183: # SIGTERM instead. TODO(jmesserly): make this more robust On 2012/02/21 18:19:35, Emily Fortuna wrote: > FYI: I've filed a feature request about this here: > https://code.google.com/p/dart/issues/detail?id=1756 > Maybe add issue # in comment here? Done. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:216: print '>>> TEST PASS' On 2012/02/21 18:19:35, Emily Fortuna wrote: > I believe you need to print out the following "magic string" if a test passed: > 'Content-Type: text/plain\nPASS' > (see line 164 for example). This is printed still by report_results. But I'm not sure if we need to keep that, since ">>> TEST PASS" is the magic string used by the BatchRunnerProcess?
lgtm, with changes in comments! https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:102: List<String> get batchRunnerArguments() => ['-batch']; On 2012/02/21 18:58:43, John Messerly wrote: > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > Why use one dash here (-batch) and two dashes below (--batch)? > > I didn't want to change the DartC test runner. Likewise, I don't think Python's > option parser library will handle a single "-". It's ugly but I'm not sure what > else to do. Ah, okay. Carry on! https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:260: RunningProcess(TestCase this.testCase, this.processQueue); On 2012/02/21 18:58:43, John Messerly wrote: > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > Careful here. The VM uses the RunningProcess class in their own standalone > test > > suite. Either we need to modify their standalone test framework with the > updated > > constructor for RunningProcess, or make processQueue optional. > > Done. FYI: I know this because I made the same mistake when I added an argument to allow retrying of tests. :-) When you gclient sync, we're going to have to resolve having multiple "optional" arguments. (http://code.google.com/p/dart/source/diff?spec=svn4404&r=4372&format=side&pat...). https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:297: processQueue._getBatchRunner(testCase).startTest(testCase); probably need to add a check to make sure processQueue isn't null since it's "optional" now. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:414: // Use a graceful shutdown so our Selenium script can close browser I just reread this comment -- I think you left out some words or something got edited out. Also, :%s/send/Send https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:417: new Timer((e) { if (!closed) _process.kill(); }, 30000); On 2012/02/21 18:58:43, John Messerly wrote: > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > Where's 30000 coming from? > > Needed a timeout :) Unfortunately, we don't have access to a testCase at this > point. And it's not clear if that would be the right timeout to use, since this > is a timeout for how long we think run_selenium will take to shutdown, not how > long an individual test needs. > > I thought about storing it in a variable, but I wasn't sure that a level of > indirection adds anything. The hope was that the line was fairly self > explanatory ("in 30s, if the process is not closed, kill it") Call me a curmudgeon, but I'd like it if you made it a variable that says something to that effect (SELENIUM_SHUTDOWN_TIME). Yes, it adds a level of indirection, but it explains how you (somewhat randomly) picked the number. :-) https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_suite.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_suite.dart:537: '--browser=${configuration["browser"]}', On 2012/02/21 18:58:43, John Messerly wrote: > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > Not that it really matters, but why did we change the order these arguments > are > > passed here? > > At one point I thought I needed --browser first so that could be configured at > startup of run_selenium. The idea is the first 2 args would be part of the > "batch mode" command line, and the last 2 args would be part of the "per test" > command line. I ended up making it handle the --browser though, so this could be > reverted. WDYT? Either way is fine. This works for me. I don't have a strong preference one way or the other. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... File tools/testing/run_selenium.py (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/run_se... tools/testing/run_selenium.py:216: print '>>> TEST PASS' On 2012/02/21 18:58:43, John Messerly wrote: > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > I believe you need to print out the following "magic string" if a test passed: > > 'Content-Type: text/plain\nPASS' > > (see line 164 for example). > > This is printed still by report_results. But I'm not sure if we need to keep > that, since ">>> TEST PASS" is the magic string used by the BatchRunnerProcess? Oh, I see. Seems fine then.
Thanks! I'll get to work on merging. Of course the real tests will be: does this actually work on the bots :). If it makes things less stable we can revert. About processQueue, the key question in my mind is: can it be null when configuration == webdriver. If not, then we really just want an assertion inside that "if" branch (and fortunately doing "processQueue." gives us that assertion for free. Hooray memory safe languages ;) ) If VM tests try to run in configuration == webdriver, then I'm not sure what to do. We could fall back to single run mode, but that doesn't seem good either, because it means webdriver tests will be run inconsistently. I'll see if I can reproduce that situation locally and figure out what happens when those two flags are merged. If the user can cause it from the command line, then at least issuing a nice exception message seems required. https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:414: // Use a graceful shutdown so our Selenium script can close browser On 2012/02/21 19:35:33, Emily Fortuna wrote: > I just reread this comment -- I think you left out some words or something got > edited out. Also, :%s/send/Send fixed. Side note: what's the deal with this capitalization rule? I haven't seen that enforced in any CL previously. It seems a bit strict--especially since the rules for capitalization after a colon isn't even agreed upon in English: http://en.wikipedia.org/wiki/Colon_(punctuation)#Use_of_capitals https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:417: new Timer((e) { if (!closed) _process.kill(); }, 30000); On 2012/02/21 19:35:33, Emily Fortuna wrote: > On 2012/02/21 18:58:43, John Messerly wrote: > > On 2012/02/21 18:19:35, Emily Fortuna wrote: > > > Where's 30000 coming from? > > > > Needed a timeout :) Unfortunately, we don't have access to a testCase at this > > point. And it's not clear if that would be the right timeout to use, since > this > > is a timeout for how long we think run_selenium will take to shutdown, not how > > long an individual test needs. > > > > I thought about storing it in a variable, but I wasn't sure that a level of > > indirection adds anything. The hope was that the line was fairly self > > explanatory ("in 30s, if the process is not closed, kill it") > > Call me a curmudgeon, but I'd like it if you made it a variable that says > something to that effect (SELENIUM_SHUTDOWN_TIME). Yes, it adds a level of > indirection, but it explains how you (somewhat randomly) picked the number. :-) Added a comment to that effect. Also moved the timer line down, since the "graceful shutdown" comment applies to the "--terminate".
re: your processQueue message at the beginning: Sounds fine to me. lgtm! https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/2001/tools/testing/dart/t... tools/testing/dart/test_runner.dart:414: // Use a graceful shutdown so our Selenium script can close browser Ah, perhaps, I'm being overzealous in applying capitalization. I was following this: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen.... In the TODO section, it doesn't explicitly say the first word of the TODO has to be capitalized, but the examples do so: http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=TODO_C.... ps. Fun sidenote, enjoy the example returns in FetchBigtableRows from the first link. On 2012/02/21 21:29:56, John Messerly wrote: > On 2012/02/21 19:35:33, Emily Fortuna wrote: > > I just reread this comment -- I think you left out some words or something got > > edited out. Also, :%s/send/Send > > fixed. > > Side note: what's the deal with this capitalization rule? I haven't seen that > enforced in any CL previously. It seems a bit strict--especially since the rules > for capitalization after a colon isn't even agreed upon in English: > http://en.wikipedia.org/wiki/Colon_%28punctuation%29#Use_of_capitals
We came across a potential problem when diagnosing a bug in co19 tests running on dartc. https://chromiumcodereview.appspot.com/9420037/diff/10003/tools/testing/dart/... File tools/testing/dart/test_runner.dart (right): https://chromiumcodereview.appspot.com/9420037/diff/10003/tools/testing/dart/... tools/testing/dart/test_runner.dart:392: _executable = testCase.executablePath; Why isn't there an assignment to _batchArguments here? If it is needed in line 400, it should also be needed here. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
