Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2551273002:
Fixed flaky SadTabReloadButton test (Closed)
DescriptionFixed flaky SadTabReloadButton test
This error was caused by the SadTab getting the Sad Tab button label too
quickly. We needed to wait for the full load to finish before pulling
the button.
BUG=670920
Committed: https://crrev.com/a4d82dd921d35ea42273aca69d43c290046b42e1
Cr-Commit-Position: refs/heads/master@{#436772}
Patch Set 1 #Patch Set 2 : Fixed flaky SadTabReloadButton test #
Total comments: 8
Patch Set 3 : Updated off twellington and tedchoc's comments #Patch Set 4 : Updated off twellington and tedchoc's comments #
Total comments: 2
Patch Set 5 : Updated off twellington and tedchoc's comments #Messages
Total messages: 30 (16 generated)
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Description was changed from ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. Fixing sadtab test BUG=670920 ========== to ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. BUG=670920 ==========
jwanda@chromium.org changed reviewers: + twellington@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jwanda@chromium.org changed reviewers: + tedchoc@chromium.org
You both have the best knowledge of the Sad Tab project who also have Owners. When you have the time feel free to check the new test!
https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:87: ChromeTabUtils.waitForTabPageLoaded(tab, new Runnable() { I think you can replace this try/catch block with just: loadUrl from ChromeActivityTestCaseBase. https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:101: } maybe add an: assertFalse(tab.isShowingSadTab()); here? https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:103: actualText = getSadTabButton(tab).getText().toString(); One thing to consider is that you're getting the text of the button on the instrumentation test thread while it is being updated on the UI thread. In general, that is likely fine due to the synchronous operation of simulateRendererKilled running on the ui thread blocking. If you do find this to be an issue, you can get the text on the Ui thread using ThreadUtils, or you can use Criteria.equals to poll for you. But in general, I don't think you actually need to do anything here.
lgtm % test name comment https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:68: public void testChangeSadButtonToFeedbackAfterFailedRefreshRevertsBackToReloadAfterSuccess() { While very descriptive, this test name is a bit of a mouthful. I would call it something like testSadTabPageButtonText since the description explains in more detail what is being tested.
Alright fixed everything, will upload as long as it passes all the tests. https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:68: public void testChangeSadButtonToFeedbackAfterFailedRefreshRevertsBackToReloadAfterSuccess() { On 2016/12/06 00:44:28, Theresa Wellington wrote: > While very descriptive, this test name is a bit of a mouthful. I would call it > something like testSadTabPageButtonText since the description explains in more > detail what is being tested. Done. https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:87: ChromeTabUtils.waitForTabPageLoaded(tab, new Runnable() { On 2016/12/06 00:44:07, Ted C wrote: > I think you can replace this try/catch block with just: > > loadUrl from ChromeActivityTestCaseBase. You're absolutely right. https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:101: } On 2016/12/06 00:44:08, Ted C wrote: > maybe add an: > assertFalse(tab.isShowingSadTab()); > here? Done. https://codereview.chromium.org/2551273002/diff/10002/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:103: actualText = getSadTabButton(tab).getText().toString(); On 2016/12/06 00:44:08, Ted C wrote: > One thing to consider is that you're getting the text of the button on the > instrumentation test thread while it is being updated on the UI thread. > > In general, that is likely fine due to the synchronous operation of > simulateRendererKilled running on the ui thread blocking. > > If you do find this to be an issue, you can get the text on the Ui thread using > ThreadUtils, or you can use Criteria.equals to poll for you. > > But in general, I don't think you actually need to do anything here. Interesting. I'll keep this sort of thing in mind for the future then!
It flaked again: https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... This will fix that? :)
On 2016/12/06 12:19:05, hbos_chromium wrote: > It flaked again: > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > This will fix that? :) It was supposed to be disabled yesterday while the fix was in-progress. JJ, did the CL to disable the tests land?
On 2016/12/06 16:46:39, Theresa Wellington wrote: > On 2016/12/06 12:19:05, hbos_chromium wrote: > > It flaked again: > > > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > > > This will fix that? :) > > It was supposed to be disabled yesterday while the fix was in-progress. JJ, did > the CL to disable the tests land? I flipped the commit bit so it should have? It still missed a LGTM for an owner so I'm not sure if it did.
On 2016/12/06 17:01:24, JJ wrote: > On 2016/12/06 16:46:39, Theresa Wellington wrote: > > On 2016/12/06 12:19:05, hbos_chromium wrote: > > > It flaked again: > > > > > > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > > > > > This will fix that? :) > > > > It was supposed to be disabled yesterday while the fix was in-progress. JJ, > did > > the CL to disable the tests land? > > I flipped the commit bit so it should have? It still missed a LGTM for an owner > so I'm not sure if it did. It looks like the other CL got deleted - disabling in this patch https://codereview.chromium.org/2550373003 until this CL with the fix is ready to land.
On 2016/12/06 17:13:52, Theresa Wellington wrote: > On 2016/12/06 17:01:24, JJ wrote: > > On 2016/12/06 16:46:39, Theresa Wellington wrote: > > > On 2016/12/06 12:19:05, hbos_chromium wrote: > > > > It flaked again: > > > > > > > > > > https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... > > > > > > > > This will fix that? :) > > > > > > It was supposed to be disabled yesterday while the fix was in-progress. JJ, > > did > > > the CL to disable the tests land? > > > > I flipped the commit bit so it should have? It still missed a LGTM for an > owner > > so I'm not sure if it did. > > It looks like the other CL got deleted - disabling in this patch > https://codereview.chromium.org/2550373003 until this CL with the fix is ready > to land. Apologies, I assumed I uploaded the patch yesterday and it turned out that I had not. Take a look whenever you're free!
lgtm w/ nit https://codereview.chromium.org/2551273002/diff/50001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2551273002/diff/50001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:86: e1.printStackTrace(); do you need this exception block? If it throws an exception, your test will fail anyway, so I think it would be ok to drop this.
Fixed the nit. Will upload after a dry run. https://codereview.chromium.org/2551273002/diff/50001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2551273002/diff/50001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:86: e1.printStackTrace(); On 2016/12/06 21:35:12, Ted C wrote: > do you need this exception block? If it throws an exception, your test will > fail anyway, so I think it would be ok to drop this. Correct, thanks for the tip.
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2551273002/#ps70001 (title: "Updated off twellington and tedchoc's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 70001, "attempt_start_ts": 1481065515502310,
"parent_rev": "b46a51e5683955f390b61d2358d2396e2294d8b2", "commit_rev":
"c13d2369716c6f2871ac7f7f68549e2dd9675601"}
Message was sent while issue was closed.
Description was changed from ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. BUG=670920 ========== to ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. BUG=670920 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. BUG=670920 ========== to ========== Fixed flaky SadTabReloadButton test This error was caused by the SadTab getting the Sad Tab button label too quickly. We needed to wait for the full load to finish before pulling the button. BUG=670920 Committed: https://crrev.com/a4d82dd921d35ea42273aca69d43c290046b42e1 Cr-Commit-Position: refs/heads/master@{#436772} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a4d82dd921d35ea42273aca69d43c290046b42e1 Cr-Commit-Position: refs/heads/master@{#436772} |
