Fixed flaky test

Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test

Committed: https://crrev.com/368ac3b055c023a43486a7638359fa63b30cc1fe
Cr-Commit-Position: refs/heads/master@{#436089}

[kumarniranjan, 2016-11-24 23:26:15]

Description was changed from

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test
==========

to

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test
==========

[kumarniranjan, 2016-11-24 23:26:15]

kumarniranjan@google.com changed reviewers:
+ joth@chromium.org, xiyuan@chromium.org

[xiyuan, 2016-11-28 23:38:40]

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc
(right):

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:175:
//#endif
Remove all these and MAYBE_RequiresNoInput -> RequiresNoInput

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:198:
.WaitNoAssertCurrentScreen();
Line 185 probably fix the issue. We should not get rid of the assert here.

[kumarniranjan, 2016-11-29 23:36:47]

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc
(right):

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:175:
//#endif
On 2016/11/28 23:38:40, xiyuan wrote:
> Remove all these and MAYBE_RequiresNoInput -> RequiresNoInput

Done.

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:198:
.WaitNoAssertCurrentScreen();
On 2016/11/28 23:38:40, xiyuan wrote:
> Line 185 probably fix the issue. We should not get rid of the assert here.

Putting the assert back causes the test to fail with the following error:

../../chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:25: Failure
Value of: GetOobeUI()->current_screen()
  Actual: 4-byte object <0B-00 00-00>
Expected: expected_screen_
Which is: 4-byte object <05-00 00-00>

However, we are indeed reaching the enrollment screen, because when we use
WaitNoAssertCurrentScreen, line 225 (which casts the screen object to type
EnrollmentScreen) does not run into problems.

[xiyuan, 2016-11-29 23:50:06]

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc
(right):

https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:198:
.WaitNoAssertCurrentScreen();
On 2016/11/29 23:36:47, kumarniranjan wrote:
> On 2016/11/28 23:38:40, xiyuan wrote:
> > Line 185 probably fix the issue. We should not get rid of the assert here.
> 
> Putting the assert back causes the test to fail with the following error:
> 
> ../../chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:25: Failure
> Value of: GetOobeUI()->current_screen()
>   Actual: 4-byte object <0B-00 00-00>
> Expected: expected_screen_
> Which is: 4-byte object <05-00 00-00>
> 
> However, we are indeed reaching the enrollment screen, because when we use
> WaitNoAssertCurrentScreen, line 225 (which casts the screen object to type
> EnrollmentScreen) does not run into problems.

0x0B is SCREEN_ERROR_MESSAGE, aka the network error screen that shows up when
the login ui thinks the device has no Internet connection. It could be
transient.

Line 225 works probably because the JS wait on the abe step success or error.

What would you see on screen if you add "--enable-pixel-output-in-tests" to
browser_tests's command line with a base::RunLoop().Run() at the end of the test
function?

[kumarniranjan, 2016-11-30 00:59:13]

On 2016/11/29 23:50:06, xiyuan wrote:
>
https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/screens/network_screen_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2526423002/diff/1/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/screens/network_screen_browsertest.cc:198:
> .WaitNoAssertCurrentScreen();
> On 2016/11/29 23:36:47, kumarniranjan wrote:
> > On 2016/11/28 23:38:40, xiyuan wrote:
> > > Line 185 probably fix the issue. We should not get rid of the assert here.
> > 
> > Putting the assert back causes the test to fail with the following error:
> > 
> > ../../chrome/browser/chromeos/login/test/oobe_screen_waiter.cc:25: Failure
> > Value of: GetOobeUI()->current_screen()
> >   Actual: 4-byte object <0B-00 00-00>
> > Expected: expected_screen_
> > Which is: 4-byte object <05-00 00-00>
> > 
> > However, we are indeed reaching the enrollment screen, because when we use
> > WaitNoAssertCurrentScreen, line 225 (which casts the screen object to type
> > EnrollmentScreen) does not run into problems.
> 
> 0x0B is SCREEN_ERROR_MESSAGE, aka the network error screen that shows up when
> the login ui thinks the device has no Internet connection. It could be
> transient.
> 
> Line 225 works probably because the JS wait on the abe step success or error.
> 
> What would you see on screen if you add "--enable-pixel-output-in-tests" to
> browser_tests's command line with a base::RunLoop().Run() at the end of the
test
> function?

When I do this, the UI gets stuck on the network error screen (no internet
connection) and then the test times out.
This appears to confirm what you were suspecting. How do I work around this?

[xiyuan, 2016-11-30 17:50:49]

On 2016/11/30 00:59:13, kumarniranjan wrote:
> When I do this, the UI gets stuck on the network error screen (no internet
> connection) and then the test times out.
> This appears to confirm what you were suspecting. How do I work around this?

Thank you for trying it out. Sounds like we did not mock network state enough to
satisfy the login code. We might need to figure out who is calling
ErrorScreen::Show and see if that gives us any clue. I suspect we might need
something like OobeBaseTest::SimulateNetworkOnline [1] in addition to the
existing |mock_network_state_helper_|.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/test/oobe_...

[xiyuan, 2016-12-02 23:23:09]

lgtm

Thanks.

[kumarniranjan, 2016-12-02 23:30:15]

The CQ bit was checked by kumarniranjan@google.com

[commit-bot: I haz the power, 2016-12-02 23:31:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 00:12:37]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480721415373200,
"parent_rev": "b33a155743e3313837f5a4addab2faf27a1a6b9c", "commit_rev":
"1919e57aab1380af9f5d9a31229baa4720a625df"}

[commit-bot: I haz the power, 2016-12-03 00:13:02]

Description was changed from

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test
==========

to

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test
==========

[commit-bot: I haz the power, 2016-12-03 00:13:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-03 00:14:36]

Description was changed from

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test
==========

to

==========
Fixed flaky test

The test HandsOffNetworkScreenTest.RequiresNoInput was flaky.
Made some changes to fix that.

BUG=668054
TEST=browser test

Committed: https://crrev.com/368ac3b055c023a43486a7638359fa63b30cc1fe
Cr-Commit-Position: refs/heads/master@{#436089}
==========

[commit-bot: I haz the power, 2016-12-03 00:14:37]

Patchset 5 (id:??) landed as
https://crrev.com/368ac3b055c023a43486a7638359fa63b30cc1fe
Cr-Commit-Position: refs/heads/master@{#436089}