Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2134873002:
[ios] Fixed SetPageDisplayStateWithUserScalableEnabled test for iOS 10. (Closed)
|
Created:
4 years, 5 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Fixed SetPageDisplayStateWithUserScalableEnabled test for iOS 10.
Looks like maximum zoom scale on iOS10 is 5.0, which should be set
instead of 10.0.
BUG=626688
Committed: https://crrev.com/a5f8f278c93d1601c58eccc8f84c12ad50e16cbc
Cr-Commit-Position: refs/heads/master@{#404518}
Patch Set 1 #
Messages
Total messages: 17 (4 generated)
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org
The CQ bit was checked by kkhorimoto@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rohitrao@chromium.org changed reviewers: + rohitrao@chromium.org
How did you figure this out? On my laptop, this failed by spinning in NSRunLoop forever. Do you understand why that happened, and why the timeout didn't fire? That's more concerning to me than the test failure.
On 2016/07/08 22:26:13, rohitrao wrote: > How did you figure this out? > > On my laptop, this failed by spinning in NSRunLoop forever. Do you understand > why that happened, and why the timeout didn't fire? That's more concerning to > me than the test failure. Test has been waiting for max zoom to be changed to 10, which never happened. I don't know why timeout (which is 10 seconds) did not fire, maybe there is a bug in wait_util....
On 2016/07/08 22:35:05, Eugene But wrote: > On 2016/07/08 22:26:13, rohitrao wrote: > > How did you figure this out? > > > > On my laptop, this failed by spinning in NSRunLoop forever. Do you understand > > why that happened, and why the timeout didn't fire? That's more concerning to > > me than the test failure. > Test has been waiting for max zoom to be changed to 10, which never happened. > I don't know why timeout (which is 10 seconds) did not fire, maybe there is a > bug in wait_util.... Aha, that makes sense, ok. Let's try running this test on 9.3 with a max zoom of 20, to see if the timeout fires.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [ios] Fixed SetPageDisplayStateWithUserScalableEnabled test for iOS 10. Looks like maximum zoom scale on iOS10 is 5.0, which should be set instead of 10.0. BUG=626688 ========== to ========== [ios] Fixed SetPageDisplayStateWithUserScalableEnabled test for iOS 10. Looks like maximum zoom scale on iOS10 is 5.0, which should be set instead of 10.0. BUG=626688 Committed: https://crrev.com/a5f8f278c93d1601c58eccc8f84c12ad50e16cbc Cr-Commit-Position: refs/heads/master@{#404518} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a5f8f278c93d1601c58eccc8f84c12ad50e16cbc Cr-Commit-Position: refs/heads/master@{#404518}
Message was sent while issue was closed.
On 2016/07/08 22:41:09, rohitrao wrote: > On 2016/07/08 22:35:05, Eugene But wrote: > > On 2016/07/08 22:26:13, rohitrao wrote: > > > How did you figure this out? > > > > > > On my laptop, this failed by spinning in NSRunLoop forever. Do you > understand > > > why that happened, and why the timeout didn't fire? That's more concerning > to > > > me than the test failure. > > Test has been waiting for max zoom to be changed to 10, which never happened. > > I don't know why timeout (which is 10 seconds) did not fire, maybe there is a > > bug in wait_util.... > > Aha, that makes sense, ok. > > Let's try running this test on 9.3 with a max zoom of 20, to see if the timeout > fires. I sort of feel that wait_util never failed on timeout (and I actually did not know that there was a timeout until now :) ). Bots have own timeouts, so this problem is probably exist only for local runs. I will try playing with wait_util to see why implementation broken.
Message was sent while issue was closed.
On 2016/07/08 22:46:21, Eugene But wrote: > On 2016/07/08 22:41:09, rohitrao wrote: > > On 2016/07/08 22:35:05, Eugene But wrote: > > > On 2016/07/08 22:26:13, rohitrao wrote: > > > > How did you figure this out? > > > > > > > > On my laptop, this failed by spinning in NSRunLoop forever. Do you > > understand > > > > why that happened, and why the timeout didn't fire? That's more > concerning > > to > > > > me than the test failure. > > > Test has been waiting for max zoom to be changed to 10, which never > happened. > > > I don't know why timeout (which is 10 seconds) did not fire, maybe there is > a > > > bug in wait_util.... > > > > Aha, that makes sense, ok. > > > > Let's try running this test on 9.3 with a max zoom of 20, to see if the > timeout > > fires. > I sort of feel that wait_util never failed on timeout (and I actually did not > know that there was a timeout until now :) ). Bots have own timeouts, so this > problem is probably exist only for local runs. I will try playing with wait_util > to see why implementation broken. Ok, so infinite timeout is done by design: https://cs.chromium.org/chromium/src/base/test/test_timeouts.cc?q=kAlmostInfi...
Message was sent while issue was closed.
With a max zoom of 20, the test spins forever on 9.3 as well. So it seems like WaitUtil was always broken and this isn't a regression.
Message was sent while issue was closed.
On 2016/07/08 22:59:50, rohitrao wrote: > With a max zoom of 20, the test spins forever on 9.3 as well. So it seems like > WaitUtil was always broken and this isn't a regression. 28 hours is kinda long timeout, but that seems to be intentional (according to the code). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
