Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806743004:
Fixing testBrowsingPostEntryWithButton flakiness. (Closed)
|
Created:
3 years, 8 months ago by liaoyuke Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, baxley+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, huangml+watch_chromium.org, liaoyuke+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing testBrowsingPostEntryWithButton flakiness.
This test has been flaky on devices because Earl Grey fails to
synchronize with omnibox text.
This CL first fixes the flakiness by adding a wait to wait for omnibox
text, and then this test seems to be more related to form submission,
so moving it to FormsTestCase.
BUG=
Review-Url: https://codereview.chromium.org/2806743004
Cr-Commit-Position: refs/heads/master@{#463415}
Committed: https://chromium.googlesource.com/chromium/src/+/33c5ac234dc93a3cfc31fda63fe716d8d2a40410
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments #Patch Set 3 : Remove duplicate using expression. #Patch Set 4 : Re-factoring to use helper methods #
Total comments: 4
Messages
Total messages: 41 (11 generated)
Description was changed from ========== nja # Enter a description of the change. Fixing BrowsingTestCase.testBrowsingPostEntryWithButton This test has been flaky on device because Earl Grey fails to synchronize with omnibox text. This CL first fixes the flakiness by adding a wait to wait for omnibox text, and then this test seems to be more related to form submission, so moving it to FormsTestCase. BUG= ========== to ========== Fixing testBrowsingPostEntryWithButton flakiness. This test has been flaky on devices because Earl Grey fails to synchronize with omnibox text. This CL first fixes the flakiness by adding a wait to wait for omnibox text, and then this test seems to be more related to form submission, so moving it to FormsTestCase. BUG= ==========
liaoyuke@chromium.org changed reviewers: + baxley@chromium.org, eugenebut@chromium.org
Hey Mike, Eugene, PTAL. Thank you very much!
eugenebut@chromium.org changed reviewers: + gchatz@chromium.org
Do we understand why this test fails? Is it different issue from testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix?
On 2017/04/07 22:48:22, Eugene But wrote: > Do we understand why this test fails? Is it different issue from > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? Looks like this is the same issue, just approaching the wait differently. My CL waits based on load start/completion, whereas this one waits based on when the omnibox text changes. We should probably unify approaches.
On 2017/04/07 22:50:40, gchatz wrote: > On 2017/04/07 22:48:22, Eugene But wrote: > > Do we understand why this test fails? Is it different issue from > > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? > > Looks like this is the same issue, just approaching the wait differently. My CL > waits based on load start/completion, whereas this one waits based on when the > omnibox text changes. We should probably unify approaches. Yeah, I agree that these 2 tests should follow the same approach.
On 2017/04/07 22:50:40, gchatz wrote: > On 2017/04/07 22:48:22, Eugene But wrote: > > Do we understand why this test fails? Is it different issue from > > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? > > Looks like this is the same issue, just approaching the wait differently. My CL > waits based on load start/completion, whereas this one waits based on when the > omnibox text changes. We should probably unify approaches. Difference between this test and mine is that mine disables synchronization and submits the form through the keyboard. These differences do not seem to matter.
On 2017/04/07 22:55:01, gchatz wrote: > On 2017/04/07 22:50:40, gchatz wrote: > > On 2017/04/07 22:48:22, Eugene But wrote: > > > Do we understand why this test fails? Is it different issue from > > > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? > > > > Looks like this is the same issue, just approaching the wait differently. My > CL > > waits based on load start/completion, whereas this one waits based on when the > > omnibox text changes. We should probably unify approaches. > > Difference between this test and mine is that mine disables synchronization and > submits the form through the keyboard. These differences do not seem to matter. I think I prefer waiting for the omnibox text, assuming that's the end state. My concern is we're doing waiting for things that can change between loaded and loading, and if we can avoid those changes and just wait for some state, it might be less flaky. But if I'm missing some value of the other approach, or problems with the text approach, let me know!
On 2017/04/07 23:19:20, baxley wrote: > On 2017/04/07 22:55:01, gchatz wrote: > > On 2017/04/07 22:50:40, gchatz wrote: > > > On 2017/04/07 22:48:22, Eugene But wrote: > > > > Do we understand why this test fails? Is it different issue from > > > > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? > > > > > > Looks like this is the same issue, just approaching the wait differently. My > > CL > > > waits based on load start/completion, whereas this one waits based on when > the > > > omnibox text changes. We should probably unify approaches. > > > > Difference between this test and mine is that mine disables synchronization > and > > submits the form through the keyboard. These differences do not seem to > matter. > > I think I prefer waiting for the omnibox text, assuming that's the end state. My > concern is we're doing waiting for things that can change between loaded and > loading, and if we can avoid those changes and just wait for some state, it > might be less flaky. But if I'm missing some value of the other approach, or > problems with the text approach, let me know! I agree that the less risk of flake would be to wait for omnibox text. I will close the other cl, and make one that uses the API introduced by this CL.
On 2017/04/08 00:08:09, gchatz wrote: > On 2017/04/07 23:19:20, baxley wrote: > > On 2017/04/07 22:55:01, gchatz wrote: > > > On 2017/04/07 22:50:40, gchatz wrote: > > > > On 2017/04/07 22:48:22, Eugene But wrote: > > > > > Do we understand why this test fails? Is it different issue from > > > > > testBrowsingPostEntryWithKeyboard, which gchatz is trying to fix? > > > > > > > > Looks like this is the same issue, just approaching the wait differently. > My > > > CL > > > > waits based on load start/completion, whereas this one waits based on when > > the > > > > omnibox text changes. We should probably unify approaches. > > > > > > Difference between this test and mine is that mine disables synchronization > > and > > > submits the form through the keyboard. These differences do not seem to > > matter. > > > > I think I prefer waiting for the omnibox text, assuming that's the end state. > My > > concern is we're doing waiting for things that can change between loaded and > > loading, and if we can avoid those changes and just wait for some state, it > > might be less flaky. But if I'm missing some value of the other approach, or > > problems with the text approach, let me know! > > I agree that the less risk of flake would be to wait for omnibox text. I will > close the other cl, and make one that uses the API introduced by this CL. lgtm with comment
https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:368: waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; Should |waitForOmniboxText:| take std::string instead? I feel that tests will either pass URL.GetContent() or string literal, so std::string would lead to a cleaner code. But also we have a bunch of tests that wait for web view with specific text. Should this test also wait for web view with specific text (so we don't have to add new API)?
https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_earl_grey.mm:172: text))] Could the matcher be made a local variable to have better formatting?
On 2017/04/08 00:17:05, gchatz wrote: > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... > File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... > ios/chrome/test/earl_grey/chrome_earl_grey.mm:172: text))] > Could the matcher be made a local variable to have better formatting? If we can test what we are trying to test (that a load happened to a particular) without doing the JavaScript injection to verify particular page text, that sounds better to me.
https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_earl_grey.mm:172: text))] On 2017/04/08 00:17:05, gchatz wrote: > Could the matcher be made a local variable to have better formatting? Using functions for matchers instead of local variables looks cleaner and it is more reusable. In this case changing argument to std::string and adding |using chrome_test_util::OmniboxText| would solve the formatting problem. But do we even need |waitForOmniboxText:| API? Can we wait for web view text instead?
https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:368: waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; On 2017/04/08 00:16:27, Eugene But wrote: > Should |waitForOmniboxText:| take std::string instead? I feel that tests will > either pass URL.GetContent() or string literal, so std::string would lead to a > cleaner code. > > But also we have a bunch of tests that wait for web view with specific text. > Should this test also wait for web view with specific text (so we don't have to > add new API)? Yeah, I thought about that, but I went with NSString* because I want it to be consistent with other methods in this file. One bigger question is that, wait for web view with specific text requires that the test output something to the web view, and the test may still want to verify that omnibox changed. Would it be simpler and cleaner if the test just wait for the omnibox? https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_earl_grey.mm:172: text))] On 2017/04/08 00:20:36, Eugene But wrote: > On 2017/04/08 00:17:05, gchatz wrote: > > Could the matcher be made a local variable to have better formatting? > Using functions for matchers instead of local variables looks cleaner and it is > more reusable. In this case changing argument to std::string and adding |using > chrome_test_util::OmniboxText| would solve the formatting problem. But do we > even need |waitForOmniboxText:| API? Can we wait for web view text instead? replied in another comment, will make the change if we decide that |waitForOmniboxText:| is needed.
https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... ios/chrome/browser/web/forms_egtest.mm:368: waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; On 2017/04/08 00:43:07, liaoyuke wrote: > On 2017/04/08 00:16:27, Eugene But wrote: > > Should |waitForOmniboxText:| take std::string instead? I feel that tests will > > either pass URL.GetContent() or string literal, so std::string would lead to a > > cleaner code. > > > > But also we have a bunch of tests that wait for web view with specific text. > > Should this test also wait for web view with specific text (so we don't have > to > > add new API)? > > Yeah, I thought about that, but I went with NSString* because I want it to be > consistent with other methods in this file. Should we lean towards API convenience rather that API consistency? > One bigger question is that, wait for web view with specific text requires that > the test output something to the web view, and the test may still want to verify > that omnibox changed. Would it be simpler and cleaner if the test just wait for > the omnibox? All other tests are waiting for text. Most of the tests will not be able to wait for omnibox URL, and waitForWebViewWithText has to exist. That is already established pattern and it is better to have fewer "waitFor" APIs and fewer different ways to wait for something. WDYT?
On 2017/04/08 00:55:10, Eugene But wrote: > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > File ios/chrome/browser/web/forms_egtest.mm (right): > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > ios/chrome/browser/web/forms_egtest.mm:368: > waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; > On 2017/04/08 00:43:07, liaoyuke wrote: > > On 2017/04/08 00:16:27, Eugene But wrote: > > > Should |waitForOmniboxText:| take std::string instead? I feel that tests > will > > > either pass URL.GetContent() or string literal, so std::string would lead to > a > > > cleaner code. > > > > > > But also we have a bunch of tests that wait for web view with specific text. > > > Should this test also wait for web view with specific text (so we don't have > > to > > > add new API)? > > > > Yeah, I thought about that, but I went with NSString* because I want it to be > > consistent with other methods in this file. > Should we lean towards API convenience rather that API consistency? > > > One bigger question is that, wait for web view with specific text requires > that > > the test output something to the web view, and the test may still want to > verify > > that omnibox changed. Would it be simpler and cleaner if the test just wait > for > > the omnibox? > All other tests are waiting for text. Most of the tests will not be able to wait > for omnibox URL, and waitForWebViewWithText has to exist. That is already > established pattern and it is better to have fewer "waitFor" APIs and fewer > different ways to wait for something. WDYT? I am in favor of introducing this API. The core of what is being tested is whether navigation successfully switches to the next page. If we can get this information using the omnibox text, it is better than running a JavaScript injection to get it, as that adds unnecessary complexity and is more prone to break. Other tests that do not need to verify exactly what is on a page should use this as well.
I think it depends on whether it's necessary to verify the text or not, is there anything wrong if we just have empty pages and just verify that URL has changed. If YES, then it makes no difference between waiting for omnibox and text. Otherwise, I would prefer the omnibox API. But in any case, given that we already have a big number of tests waiting for text, and it's non-trivial work to re-factor all of them. I would prefer having less APIs. Btw, the text is not injected by JS, it's set up by the http server, so maybe it's no more prone to break? though it may add unnecessary complexity. On Fri, Apr 7, 2017 at 6:03 PM <gchatz@chromium.org> wrote: > On 2017/04/08 00:55:10, Eugene But wrote: > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > File ios/chrome/browser/web/forms_egtest.mm (right): > > > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > ios/chrome/browser/web/forms_egtest.mm:368: > > waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; > > On 2017/04/08 00:43:07, liaoyuke wrote: > > > On 2017/04/08 00:16:27, Eugene But wrote: > > > > Should |waitForOmniboxText:| take std::string instead? I feel that > tests > > will > > > > either pass URL.GetContent() or string literal, so std::string would > lead > to > > a > > > > cleaner code. > > > > > > > > But also we have a bunch of tests that wait for web view with > specific > text. > > > > Should this test also wait for web view with specific text (so we > don't > have > > > to > > > > add new API)? > > > > > > Yeah, I thought about that, but I went with NSString* because I want > it to > be > > > consistent with other methods in this file. > > Should we lean towards API convenience rather that API consistency? > > > > > One bigger question is that, wait for web view with specific text > requires > > that > > > the test output something to the web view, and the test may still want > to > > verify > > > that omnibox changed. Would it be simpler and cleaner if the test just > wait > > for > > > the omnibox? > > All other tests are waiting for text. Most of the tests will not be able > to > wait > > for omnibox URL, and waitForWebViewWithText has to exist. That is already > > established pattern and it is better to have fewer "waitFor" APIs and > fewer > > different ways to wait for something. WDYT? > > I am in favor of introducing this API. The core of what is being tested is > whether navigation successfully switches to the next page. If we can get > this > information using the omnibox text, it is better than running a JavaScript > injection to get it, as that adds unnecessary complexity and is more prone > to > break. Other tests that do not need to verify exactly what is on a page > should > use this as well. > > https://codereview.chromium.org/2806743004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/08 01:22:37, liaoyuke wrote: > I think it depends on whether it's necessary to verify the text or not, is > there anything wrong if we just have empty pages and just verify that URL > has changed. If YES, then it makes no difference between waiting for > omnibox and text. Otherwise, I would prefer the omnibox API. > > But in any case, given that we already have a big number of tests waiting > for text, and it's non-trivial work to re-factor all of them. I would > prefer having less APIs. > > Btw, the text is not injected by JS, it's set up by the http server, so > maybe it's no more prone to break? though it may add unnecessary complexity. While the creation of the web page does not use JS injection, the validation that the text is present on the page does. > > On Fri, Apr 7, 2017 at 6:03 PM <mailto:gchatz@chromium.org> wrote: > > > On 2017/04/08 00:55:10, Eugene But wrote: > > > > > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > > File ios/chrome/browser/web/forms_egtest.mm (right): > > > > > > > > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > > ios/chrome/browser/web/forms_egtest.mm:368: > > > waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; > > > On 2017/04/08 00:43:07, liaoyuke wrote: > > > > On 2017/04/08 00:16:27, Eugene But wrote: > > > > > Should |waitForOmniboxText:| take std::string instead? I feel that > > tests > > > will > > > > > either pass URL.GetContent() or string literal, so std::string would > > lead > > to > > > a > > > > > cleaner code. > > > > > > > > > > But also we have a bunch of tests that wait for web view with > > specific > > text. > > > > > Should this test also wait for web view with specific text (so we > > don't > > have > > > > to > > > > > add new API)? > > > > > > > > Yeah, I thought about that, but I went with NSString* because I want > > it to > > be > > > > consistent with other methods in this file. > > > Should we lean towards API convenience rather that API consistency? > > > > > > > One bigger question is that, wait for web view with specific text > > requires > > > that > > > > the test output something to the web view, and the test may still want > > to > > > verify > > > > that omnibox changed. Would it be simpler and cleaner if the test just > > wait > > > for > > > > the omnibox? > > > All other tests are waiting for text. Most of the tests will not be able > > to > > wait > > > for omnibox URL, and waitForWebViewWithText has to exist. That is already > > > established pattern and it is better to have fewer "waitFor" APIs and > > fewer > > > different ways to wait for something. WDYT? > > > > I am in favor of introducing this API. The core of what is being tested is > > whether navigation successfully switches to the next page. If we can get > > this > > information using the omnibox text, it is better than running a JavaScript > > injection to get it, as that adds unnecessary complexity and is more prone > > to > > break. Other tests that do not need to verify exactly what is on a page > > should > > use this as well. > > > > https://codereview.chromium.org/2806743004/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I see, thanks for letting me know this! Let's discuss more about this next Monday! On Fri, Apr 7, 2017 at 6:30 PM <gchatz@chromium.org> wrote: > On 2017/04/08 01:22:37, liaoyuke wrote: > > I think it depends on whether it's necessary to verify the text or not, > is > > there anything wrong if we just have empty pages and just verify that URL > > has changed. If YES, then it makes no difference between waiting for > > omnibox and text. Otherwise, I would prefer the omnibox API. > > > > But in any case, given that we already have a big number of tests waiting > > for text, and it's non-trivial work to re-factor all of them. I would > > prefer having less APIs. > > > > Btw, the text is not injected by JS, it's set up by the http server, so > > maybe it's no more prone to break? though it may add unnecessary > complexity. > > While the creation of the web page does not use JS injection, the > validation > that the text is present on the page does. > > > > > On Fri, Apr 7, 2017 at 6:03 PM <mailto:gchatz@chromium.org> wrote: > > > > > On 2017/04/08 00:55:10, Eugene But wrote: > > > > > > > > > > > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > > > File ios/chrome/browser/web/forms_egtest.mm (right): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2806743004/diff/1/ios/chrome/browser/web/form... > > > > ios/chrome/browser/web/forms_egtest.mm:368: > > > > > waitForOmniboxText:base::SysUTF8ToNSString(destinationURL.GetContent())]; > > > > On 2017/04/08 00:43:07, liaoyuke wrote: > > > > > On 2017/04/08 00:16:27, Eugene But wrote: > > > > > > Should |waitForOmniboxText:| take std::string instead? I feel > that > > > tests > > > > will > > > > > > either pass URL.GetContent() or string literal, so std::string > would > > > lead > > > to > > > > a > > > > > > cleaner code. > > > > > > > > > > > > But also we have a bunch of tests that wait for web view with > > > specific > > > text. > > > > > > Should this test also wait for web view with specific text (so we > > > don't > > > have > > > > > to > > > > > > add new API)? > > > > > > > > > > Yeah, I thought about that, but I went with NSString* because I > want > > > it to > > > be > > > > > consistent with other methods in this file. > > > > Should we lean towards API convenience rather that API consistency? > > > > > > > > > One bigger question is that, wait for web view with specific text > > > requires > > > > that > > > > > the test output something to the web view, and the test may still > want > > > to > > > > verify > > > > > that omnibox changed. Would it be simpler and cleaner if the test > just > > > wait > > > > for > > > > > the omnibox? > > > > All other tests are waiting for text. Most of the tests will not be > able > > > to > > > wait > > > > for omnibox URL, and waitForWebViewWithText has to exist. That is > already > > > > established pattern and it is better to have fewer "waitFor" APIs and > > > fewer > > > > different ways to wait for something. WDYT? > > > > > > I am in favor of introducing this API. The core of what is being > tested is > > > whether navigation successfully switches to the next page. If we can > get > > > this > > > information using the omnibox text, it is better than running a > JavaScript > > > injection to get it, as that adds unnecessary complexity and is more > prone > > > to > > > break. Other tests that do not need to verify exactly what is on a page > > > should > > > use this as well. > > > > > > https://codereview.chromium.org/2806743004/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2806743004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> I am in favor of introducing this API. The core of what is being tested is > whether navigation successfully switches to the next page. If we can get this > information using the omnibox text, it is better than running a JavaScript > injection to get it, as that adds unnecessary complexity and is more prone to > break. Other tests that do not need to verify exactly what is on a page should > use this as well. In most cases other tests actually want to verify exactly what is on a page and forms tests are not exception here. "Wait" APIs are pretty bad from EG framework perspective and I think we should have fewer of them.
Patchset #2 (id:20001) has been deleted
PTAL. Thank you very much!
lgtm!
PTAL! It turns out that FormsTestCase already has a global HTTP Server and a bunch of helper methods that are ready to use, so I'm changing this CL to use them and be consistent with other tests in this test case. I will create a separate CL to refacor this file and clean it up a little bit.
https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] Should this use [self waitForExpectedResponse]?
https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] On 2017/04/10 18:43:23, gchatz wrote: > Should this use [self waitForExpectedResponse]? I think we should remove this method (in a separate CL). WebViewContainingText is a matcher that waits, and it will be refactored to [ChromeEarlGrey waitForWebViewContainingText] some time in the future, so removing [self waitForExpectedResponse] will make things easier and cleaner.
lgtm https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] On 2017/04/10 18:48:09, liaoyuke wrote: > On 2017/04/10 18:43:23, gchatz wrote: > > Should this use [self waitForExpectedResponse]? > > I think we should remove this method (in a separate CL). > > WebViewContainingText is a matcher that waits, and it will be refactored to > [ChromeEarlGrey waitForWebViewContainingText] some time in the future, so > removing [self waitForExpectedResponse] will make things easier and cleaner. Ok sounds good
Still lgtm https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... File ios/chrome/browser/web/forms_egtest.mm (right): https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] On 2017/04/10 18:48:09, liaoyuke wrote: > On 2017/04/10 18:43:23, gchatz wrote: > > Should this use [self waitForExpectedResponse]? > > I think we should remove this method (in a separate CL). Could you please file a bug and add a TODO for this. Unless you plan a follow up CL.
I'll have a follow up CL to clean this test case up. On Mon, Apr 10, 2017 at 1:46 PM <eugenebut@chromium.org> wrote: > Still lgtm > > > > > https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... > File ios/chrome/browser/web/forms_egtest.mm (right): > > > https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... > ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey > selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] > On 2017/04/10 18:48:09, liaoyuke wrote: > > On 2017/04/10 18:43:23, gchatz wrote: > > > Should this use [self waitForExpectedResponse]? > > > > I think we should remove this method (in a separate CL). > Could you please file a bug and add a TODO for this. Unless you plan a > follow up CL. > > https://codereview.chromium.org/2806743004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you all for reviewing! On Mon, Apr 10, 2017 at 1:52 PM Yuke Liao <liaoyuke@chromium.org> wrote: > I'll have a follow up CL to clean this test case up. > > On Mon, Apr 10, 2017 at 1:46 PM <eugenebut@chromium.org> wrote: > > Still lgtm > > > > > https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... > File ios/chrome/browser/web/forms_egtest.mm (right): > > > https://codereview.chromium.org/2806743004/diff/80001/ios/chrome/browser/web/... > ios/chrome/browser/web/forms_egtest.mm:358: [[EarlGrey > selectElementWithMatcher:WebViewContainingText(kExpectedPostData)] > On 2017/04/10 18:48:09, liaoyuke wrote: > > On 2017/04/10 18:43:23, gchatz wrote: > > > Should this use [self waitForExpectedResponse]? > > > > I think we should remove this method (in a separate CL). > Could you please file a bug and add a TODO for this. Unless you plan a > follow up CL. > > https://codereview.chromium.org/2806743004/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by liaoyuke@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 liaoyuke@chromium.org
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": 80001, "attempt_start_ts": 1491861543439500,
"parent_rev": "4784619fba45abbf811939fe143c1378fe003246", "commit_rev":
"33c5ac234dc93a3cfc31fda63fe716d8d2a40410"}
Message was sent while issue was closed.
Description was changed from ========== Fixing testBrowsingPostEntryWithButton flakiness. This test has been flaky on devices because Earl Grey fails to synchronize with omnibox text. This CL first fixes the flakiness by adding a wait to wait for omnibox text, and then this test seems to be more related to form submission, so moving it to FormsTestCase. BUG= ========== to ========== Fixing testBrowsingPostEntryWithButton flakiness. This test has been flaky on devices because Earl Grey fails to synchronize with omnibox text. This CL first fixes the flakiness by adding a wait to wait for omnibox text, and then this test seems to be more related to form submission, so moving it to FormsTestCase. BUG= Review-Url: https://codereview.chromium.org/2806743004 Cr-Commit-Position: refs/heads/master@{#463415} Committed: https://chromium.googlesource.com/chromium/src/+/33c5ac234dc93a3cfc31fda63fe7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/33c5ac234dc93a3cfc31fda63fe7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
