Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1859523002:
Start browsers sequentially in testing scripts, as needed. (Closed)
|
Created:
4 years, 8 months ago by Bill Hesse Modified:
4 years, 8 months ago Reviewers:
ahe CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/sdk.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionStart browsers sequentially in testing scripts, as needed.
BUG=https://github.com/dart-lang/sdk/issues/26060
R=ahe@google.com
Committed: https://github.com/dart-lang/sdk/commit/7407ecded4c5ad884a3708d6a0e31b05155e0f83
Patch Set 1 #Patch Set 2 : Fix problems #
Total comments: 26
Patch Set 3 : Address comments, rename things #Patch Set 4 : Refactor per discussions #Patch Set 5 : Remove "requestBrowser" call from removeBrowser. #
Total comments: 24
Patch Set 6 : Address comments #
Messages
Total messages: 8 (2 generated)
whesse@google.com changed reviewers: + ahe@google.com
https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... File tools/testing/dart/browser_controller.dart (right): https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:856: // If the test has been empty within this period, don't start another browser. Add newline before comment to make it obvious where the comment applies. Also, shouldn't it be a documentation comment (///)? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:867: int browserIdCounter = 0; Isn't this a count, not a counter. A counter is something that ticks up itself, a count is a number (an integer). https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:933: void requestBrowser() { Who calls this method? Why and when? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:934: if (!testServerStarted) return; Why is it OK to not start a browser if the test server isn't started? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:935: if (underTermination) return; Who would be requesting a browser during termination? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:937: if (startingBrowserId != null) return; What does it mean if startingBrowserId is null? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:945: void startBrowser() { How is this different from Browser.startBrowser? Can this be reflected in a different name? Using the same method names is confusing. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:947: print("startBrowser called: $id"); Debug message? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:965: print("Started $id $numBrowsers"); Debug? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:966: status.nextTestTimeout = createNextTestTimer(status); What does this do? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1080: void removeBrowser(String id) { This is confusing: remove browser seems to add a browser. What does "remove" mean? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1084: if (startingBrowserId == id) startingBrowserId = null; What does this mean? https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1167: if (startingBrowserId == status.browser.id) startingBrowserId = null; ?
https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... File tools/testing/dart/browser_controller.dart (right): https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:856: // If the test has been empty within this period, don't start another browser. On 2016/04/05 13:19:41, ahe wrote: > Add newline before comment to make it obvious where the comment applies. Also, > shouldn't it be a documentation comment (///)? Done. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:867: int browserIdCounter = 0; On 2016/04/05 13:19:42, ahe wrote: > Isn't this a count, not a counter. A counter is something that ticks up itself, > a count is a number (an integer). This is a counter, that ticks up, just to give a non-repeating id string. It does not count anything, and does not represent anything as an integer. It could start at any number, and maybe should start at 1. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:933: void requestBrowser() { On 2016/04/05 13:19:41, ahe wrote: > Who calls this method? Why and when? Added doc comment. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:934: if (!testServerStarted) return; On 2016/04/05 13:19:41, ahe wrote: > Why is it OK to not start a browser if the test server isn't started? Because 4 lines above, when the test server is finished starting, we request a browser then. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:935: if (underTermination) return; On 2016/04/05 13:19:41, ahe wrote: > Who would be requesting a browser during termination? Another browser that is requesting a new test. If the queue is nonempty, it will try starting another browser then. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:937: if (startingBrowserId != null) return; On 2016/04/05 13:19:42, ahe wrote: > What does it mean if startingBrowserId is null? It means no browser is currently starting. I considered having a separate boolean for this, but it is exactly "there is no browser starting". Added comment at the declaration. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:945: void startBrowser() { On 2016/04/05 13:19:42, ahe wrote: > How is this different from Browser.startBrowser? > Can this be reflected in a different name? Using the same method names is > confusing. Browser.startBrowser is a bad name for that function. It could be startBrowserProcess. This functions creates a browser, saves its info, and starts it on the driver page. It could be called createBrowser instead. Just changing this name, not both. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:947: print("startBrowser called: $id"); On 2016/04/05 13:19:42, ahe wrote: > Debug message? Done. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:965: print("Started $id $numBrowsers"); On 2016/04/05 13:19:42, ahe wrote: > Debug? Done. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:966: status.nextTestTimeout = createNextTestTimer(status); On 2016/04/05 13:19:42, ahe wrote: > What does this do? Added a doc comment to createNextTestTimer and createTimeoutTimer There are cases where a test, after reporting success, keeps running code that somehow destroys the driver page and keeps it from fetching a next test. This detects this rare case. We had a problem with this in the past. Now there is always a timeout on a browser, whether it is running a test, or whether it has reported a test finished but not started the new one. There is a gap - if it gets a "WAIT" message back, but then it has its own .5 second timer. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1080: void removeBrowser(String id) { On 2016/04/05 13:19:42, ahe wrote: > This is confusing: remove browser seems to add a browser. What does "remove" > mean? Remove the entries for that browserID from our data structures, like browserStatus and numBrowsers. Only startBrowser (now renamed createBrowser) adds a browser to our data structures, and only removeBrowser removes it. Creating a replacement for the removed browser, if appropriate, is the job of requestBrowser(). https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1084: if (startingBrowserId == id) startingBrowserId = null; On 2016/04/05 13:19:41, ahe wrote: > What does this mean? startingBrowserId == null when there is no browser currently starting up, but not yet ready. https://codereview.chromium.org/1859523002/diff/20001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1167: if (startingBrowserId == status.browser.id) startingBrowserId = null; On 2016/04/05 13:19:41, ahe wrote: > ? This is covered by the call to removeBrowser below. Removed.
lgtm I feel like there's a lot of opportunity to iterate on the design here, but let's not get bogged down in that. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... File tools/testing/dart/browser_controller.dart (right): https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:847: * The interface is rather simple. After starting, the runner tests Remove "The interface is rather simple." https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:860: BrowserTestingServer testingServer; What is a testingServer? https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:872: bool testServerStarted = false; What is a testServer? Is it the same as a testingServer? https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:894: // no other browser instance currently starting up. Let's say that the queue hasn't been empty recently, but you recently started a browser. Should that also delay starting a new browser? https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:921: * overwritten with handlers for this specific configuration, in that case. This comment is confusing and not balanced (missing close parenthesis). https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:929: if (browserName == 'ff') browserName = 'firefox'; This looks like a hack. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:942: ..nextTestCallBack = getNextTest; Indent all .. lines by four. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:970: ++browserIdCounter; Convert to method: String getNextBrowserId() => "BROWSER${browserIdCounter++}"; https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:971: final url = testingServer.getDriverUrl(id); Is this a String or a Uri? Add type to make it clear? https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1143: BrowserTest test = testQueue.removeLast(); There's an opportunity to create a class that represents a test queue. It would then keep track of when it was last empty. abstract class TestQueue { final List<BrowserTest> tests; bool get hasBeenEmptyRecently; BrowserTest removeLast(); void addTest(BrowserTest test); } https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1176: /// This creates a timer that is active while a test is running on a browser. Remove "this" which is implied. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1182: /// This creates a timer that is active while no test is running on the Ditto. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1207: void queueTest(BrowserTest test) { enqueueTest?
Description was changed from ========== Start browsers sequentially in testing scripts, as needed. BUG=https://github.com/dart-lang/sdk/issues/26060 ========== to ========== Start browsers sequentially in testing scripts, as needed. BUG=https://github.com/dart-lang/sdk/issues/26060 R=ahe@google.com Committed: https://github.com/dart-lang/sdk/commit/7407ecded4c5ad884a3708d6a0e31b05155e0f83 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 7407ecded4c5ad884a3708d6a0e31b05155e0f83 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... File tools/testing/dart/browser_controller.dart (right): https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:847: * The interface is rather simple. After starting, the runner tests On 2016/04/05 16:14:08, ahe wrote: > Remove "The interface is rather simple." Done. Added info about starting browsers and starting test server. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:860: BrowserTestingServer testingServer; On 2016/04/05 16:14:07, ahe wrote: > What is a testingServer? Added to class documentation. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:872: bool testServerStarted = false; On 2016/04/05 16:14:08, ahe wrote: > What is a testServer? Is it the same as a testingServer? Changed to testingServerStarted. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:894: // no other browser instance currently starting up. On 2016/04/05 16:14:08, ahe wrote: > Let's say that the queue hasn't been empty recently, but you recently started a > browser. Should that also delay starting a new browser? No, except that a new browser shouldn't start until the previously started one requests a test (and gets it, so the queue is more likely to be empty if there are now enough browsers). https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:921: * overwritten with handlers for this specific configuration, in that case. On 2016/04/05 16:14:07, ahe wrote: > This comment is confusing and not balanced (missing close parenthesis). Rewritten https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:929: if (browserName == 'ff') browserName = 'firefox'; On 2016/04/05 16:14:07, ahe wrote: > This looks like a hack. It is. I recall that there was a failure if we normalized the name at the beginning of the program, in test_options, which is why this is local. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:942: ..nextTestCallBack = getNextTest; On 2016/04/05 16:14:07, ahe wrote: > Indent all .. lines by four. Done. Will be reformatted by the follow-up autoformatting CL. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:971: final url = testingServer.getDriverUrl(id); On 2016/04/05 16:14:08, ahe wrote: > Is this a String or a Uri? Add type to make it clear? Done. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1176: /// This creates a timer that is active while a test is running on a browser. On 2016/04/05 16:14:07, ahe wrote: > Remove "this" which is implied. Done. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1182: /// This creates a timer that is active while no test is running on the On 2016/04/05 16:14:08, ahe wrote: > Ditto. Done. https://codereview.chromium.org/1859523002/diff/80001/tools/testing/dart/brow... tools/testing/dart/browser_controller.dart:1207: void queueTest(BrowserTest test) { On 2016/04/05 16:14:08, ahe wrote: > enqueueTest? Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
