Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1433123002:
Ensure javascript tests are run "backlog style", most recent to oldest. (Closed)
DescriptionEnsure javascript tests are run "backlog style", most recent to oldest.
This fixes that flaky test tracing.ui.timeline_viewport_test.locationObj and potentially other similarly flaky tests by ensuring the test UI is visible in the content area when the test runs.
If a test is very tall or the content area window is small then the problem could come back. A separate fix to the test bot ensures the browser window has a constant fixed size which should alleviate that problem further.
BUG=catapult:#1745
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/81ef865aa6a003d4f5f76f0904ab2fad3dca7158
Patch Set 1 #Patch Set 2 : New approach, display most recent test on top. #
Total comments: 11
Patch Set 3 : Answered Nat. #Patch Set 4 : REALLY answered Nat. #
Messages
Total messages: 18 (5 generated)
beaudoin@chromium.org changed reviewers: + nduca@chromium.org
Nat: I found a way to repro consistently on my machine. By tracing I found the problem was that the track's boundingBox.top changed when the test didn't expect it to. I'm not sure if it's the test catching a real problem, or if the test writer (Dan, according to blame) didn't plan for some ways in which expanding a track may cause boundingBox.top to change (maybe because scrollbar appears?) Anyway, this patch fixes the flake if that's all we're interested in.
On 2015/11/11 01:46:49, beaudoin wrote: > Nat: I found a way to repro consistently on my machine. By tracing I found the > problem was that the track's boundingBox.top changed when the test didn't expect > it to. I'm not sure if it's the test catching a real problem, or if the test > writer (Dan, according to blame) didn't plan for some ways in which expanding a > track may cause boundingBox.top to change (maybe because scrollbar appears?) > > Anyway, this patch fixes the flake if that's all we're interested in. As noted on the bug, the behavior is that: asyncTrack.getBoundingClientRect().top increases by ~15 pixels after asyncTrack.expanded = true. In fact every element up to x-base-interactive-test-runner jump by 15 pixels. With a small browser window I see: x-base-interactive-test-runner.getBoundingClientRect().top = -28 (before the track is expanded) x-base-interactive-test-runner.getBoundingClientRect().top = -13 (after the track is expanded) I've sunk quite a bit of time investigating this and I can't really explain why this is happening. My current theory is that it's due to some weird relayout that the browser performs after the track is expanded. I can trigger that relayout earlier, but if I do the current "hack" of using scrollIntoView() to fix Location.fromViewCoordinates stops working. Given that I suggest we apply the fix in that CL. I can update the comment to reflect my current understanding of the issue.
Hmm! Sorry this is such a time sink... flake like this is often the hardest to resolve, because the flake often is really mysterious. As someone pulls at the issue, you always do uncover the easy fix right about at that point where you're sick to death of the problem and just move on. And yet, even as one proposes that easy fix, there's this general feeling amongst everyone of "guhh there's gotta be something deeper going on here." I see that here. I agree we could land this and be done, but the thing to note is that we have about 4 or so flake bugs that all feel similar to this, where something related to the addHTMLOutput goes wrong and the tests fail. Only in canary. This makes me think that if we temporary fix this, we'll just see the same issue elsewhere. This makes me conficted about how to proceed. On one hand, I do want to land this, and free you of this beast. On the other hand, I feel like there's something lurking in the deep. I wish I could repro this somehow... Ima try on my linux box in a momment, can't get my my instincts say that there is flexbox issue thats causing elements to shrink when their required size grows. The thing to know is that if you have a flexbox div, and one of its children gets too big, then in some configurations, the flexbox will shrink all the children equally to keep its dimension. When we set isExpanded, we're causing the requested height of the test element to increase, and I wonder if that then causes this shrinking... What I'd probalby do if I was chasing this is I'd look at the flex options of the parents of the html element after its added, and see if any of them have flex 1 1 auto.... that's an alias for flex-grow:1, flex-shrink:1, flex-basis: auto. I might then change the flex-shrink to 0 and see if it affected things....
On 2015/11/12 17:03:34, nduca wrote:
> Hmm!
>
> Sorry this is such a time sink... flake like this is often the hardest to
> resolve, because the flake often is really mysterious. As someone pulls at the
> issue, you always do uncover the easy fix right about at that point where
you're
> sick to death of the problem and just move on. And yet, even as one proposes
> that easy fix, there's this general feeling amongst everyone of "guhh there's
> gotta be something deeper going on here."
>
>
> I see that here. I agree we could land this and be done, but the thing to note
> is that we have about 4 or so flake bugs that all feel similar to this, where
> something related to the addHTMLOutput goes wrong and the tests fail. Only in
> canary. This makes me think that if we temporary fix this, we'll just see the
> same issue elsewhere.
>
> This makes me conficted about how to proceed. On one hand, I do want to land
> this, and free you of this beast.
>
> On the other hand, I feel like there's something lurking in the deep.
>
> I wish I could repro this somehow... Ima try on my linux box in a momment,
can't
> get my my instincts say that there is flexbox issue thats causing elements to
> shrink when their required size grows. The thing to know is that if you have a
> flexbox div, and one of its children gets too big, then in some
configurations,
> the flexbox will shrink all the children equally to keep its dimension. When
we
> set isExpanded, we're causing the requested height of the test element to
> increase, and I wonder if that then causes this shrinking...
>
> What I'd probalby do if I was chasing this is I'd look at the flex options of
> the parents of the html element after its added, and see if any of them have
> flex 1 1 auto.... that's an alias for flex-grow:1, flex-shrink:1, flex-basis:
> auto. I might then change the flex-shrink to 0 and see if it affected
things....
I can spend a bit more time on this. I'll dig more into the flex idea, but can't
see any height change so I'm not sure it's this.
Here's maybe a clearer exposition of what I'm seeing:
- Upon scrollIntoView() the top-level element <x-base-interactive-test-runner>
moves out of the screen. Its |top| goes from 0 to -32 (that number varies based
on height of my browser window).
- It's as if it was scrolled but: (1) it doesn't have a scroll bar; (2) its
scrollTop remains 0.
- Its height remains unchanged, it's the full height of the content area.
(Its CSS height is 100%, with display: flex; flex-direction: column;)
- Visually, the content is cropped, as if the full page was scrolled down.
- Upon asyncTrack.expanded the top-level element's |top| goes from -32 to -19.
- <x-base-interactive-test-runner>'s |bottom| is always equal to |top| +
|height|. Since |top| is negative and |height| is unchanged, this means |bottom|
is smaller than the content area's height. However, visually the element covers
the entire content area.
Does anybody know of a feature (or bug?) in the web platform that could cause
this "looks like scrolling but scrollTop is unchanged" behavior?
Patchset #2 (id:20001) has been deleted
Ok, Nat, I fixed this in a more generic way by displaying the most recent test at the top of the list. Making the UI a "backlog" of tests, which incidentally makes it more interesting to watch since you always see the currently running test. Ensuring the currently running test is visible by default is going to significantly reduce the number of flakes. However, I'm not sure there's anything we can do to fix all possible ways in which a test could flake. Maybe forcing all tests to run in a fixed height browser? The reason is that some of our code rely on document.elementFromPoint which will only work if the DOM element is visible in the content area. Using scrollIntoView() can ensure the top line of pixel of an element is becomes visible but: 1) A scrolled element seems to behave weirdly when its children are resized, which may be fixable but I couldn't find how (the cause of locationObj's failure). 2) After scrolling, some tests (may?) peek at pixels that are not on the top line of the element, but that happen to be visible at the current/standard browser window height. However these could fail with a smaller browser window.
https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... File tracing/tracing/base/unittest/html_test_results.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:15: x-tr-b-unittest-test-results { How come this didn't spit an error!?
https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... File tracing/tracing/base/unittest/html_test_results.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:15: x-tr-b-unittest-test-results { woah! this seems important. as does the lack of flex: 0 0 auto on the individual result objects. i wonder if you could peel a patch off that just fixes those two items? it may not deflake but it's a gaping hole worth patching while we debate the insertBefore change below... https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:360: this.insertBefore(this.currentTestCaseResult_, this.firstChild); Oh, hmm! this the critical part huh! I'm trying to grok if the issue here is that something is scrolling into view, or if something's height is shrinking... my mental model was something shrinking but if stuff is scrolling when we don't expect it, then indeed maybe you're on the right track. Hmmm....
https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... File tracing/tracing/base/unittest/html_test_results.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:15: x-tr-b-unittest-test-results { On 2015/11/13 18:24:53, nduca wrote: > woah! this seems important. as does the lack of flex: 0 0 auto on the individual > result objects. i wonder if you could peel a patch off that just fixes those two > items? it may not deflake but it's a gaping hole worth patching while we debate > the insertBefore change below... Sure, let me do that right away. https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:360: this.insertBefore(this.currentTestCaseResult_, this.firstChild); On 2015/11/13 18:24:53, nduca wrote: > Oh, hmm! this the critical part huh! I'm trying to grok if the issue here is > that something is scrolling into view, or if something's height is shrinking... > my mental model was something shrinking but if stuff is scrolling when we don't > expect it, then indeed maybe you're on the right track. Hmmm.... There definitely is something weird going on with scrollTop and I've checked suspects for height changes and haven't seen anything unexpected. I'll write a bit of code to ensure the heights of everyone in the hierarchy behaves as expected.
On 2015/11/13 19:08:25, beaudoin wrote: > https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... > File tracing/tracing/base/unittest/html_test_results.html (right): > > https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... > tracing/tracing/base/unittest/html_test_results.html:15: > x-tr-b-unittest-test-results { > On 2015/11/13 18:24:53, nduca wrote: > > woah! this seems important. as does the lack of flex: 0 0 auto on the > individual > > result objects. i wonder if you could peel a patch off that just fixes those > two > > items? it may not deflake but it's a gaping hole worth patching while we > debate > > the insertBefore change below... > > Sure, let me do that right away. > > https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... > tracing/tracing/base/unittest/html_test_results.html:360: > this.insertBefore(this.currentTestCaseResult_, this.firstChild); > On 2015/11/13 18:24:53, nduca wrote: > > Oh, hmm! this the critical part huh! I'm trying to grok if the issue here is > > that something is scrolling into view, or if something's height is > shrinking... > > my mental model was something shrinking but if stuff is scrolling when we > don't > > expect it, then indeed maybe you're on the right track. Hmmm.... > > There definitely is something weird going on with scrollTop and I've checked > suspects for height changes and haven't seen anything unexpected. I'll write a > bit of code to ensure the heights of everyone in the hierarchy behaves as > expected. Nat: Friendly ping. I still recommend landing this patch to "run the tests backlog style". Updated the desc to match. I really dont see the flakiness coming back in this setup. If it does then I suggest we have a serious discussion regarding tests simulating clicks: - Whether we can make them unflaky - If not, whether we need them, and if we do how to setup a more robust framework for such tests.
Description was changed from ========== Fixing flaky test tracing.ui.timeline_viewport_test.locationObj BUG=catapult:#1745 ========== to ========== Ensure javascript tests are run "backlog style", most recent to oldest. This fixes that flaky test tracing.ui.timeline_viewport_test.locationObj and potentially other similarly flaky tests by ensuring the test UI is visible in the content area when the test runs. If a test is very tall or the content area window is small then the problem could come back. A separate fix to the test bot ensures the browser window has a constant fixed size which should alleviate that problem further. BUG=catapult:#1745 ==========
lgtm with a minor suggestion https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... File tracing/tracing/base/unittest/html_test_results.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:15: x-tr-b-unittest-test-results { i assume rebasing will cause this edit to go away? https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:360: this.insertBefore(this.currentTestCaseResult_, this.firstChild); this approach sounds good. could you add a reverse to the test array in the interactive test runner though that causes it to run tests in reverse-alphabetical order s.t. things end up sorted in the final view? https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/ui/time... File tracing/tracing/ui/timeline_viewport_test.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/ui/time... tracing/tracing/ui/timeline_viewport_test.html:79: tr.c.TestUtils.newAsyncSliceNamed('a', 80, 20, thread, thread)); want to remove this edit? its a separate patch and it confuses the diff.
https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... File tracing/tracing/base/unittest/html_test_results.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:15: x-tr-b-unittest-test-results { On 2015/11/18 20:38:08, nduca wrote: > i assume rebasing will cause this edit to go away? Done. https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/base/un... tracing/tracing/base/unittest/html_test_results.html:360: this.insertBefore(this.currentTestCaseResult_, this.firstChild); On 2015/11/18 20:38:08, nduca wrote: > this approach sounds good. could you add a reverse to the test array in the > interactive test runner though that causes it to run tests in > reverse-alphabetical order s.t. things end up sorted in the final view? Done. https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/ui/time... File tracing/tracing/ui/timeline_viewport_test.html (right): https://codereview.chromium.org/1433123002/diff/40001/tracing/tracing/ui/time... tracing/tracing/ui/timeline_viewport_test.html:79: tr.c.TestUtils.newAsyncSliceNamed('a', 80, 20, thread, thread)); On 2015/11/18 20:38:09, nduca wrote: > want to remove this edit? its a separate patch and it confuses the diff. Done.
The CQ bit was checked by beaudoin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org Link to the patchset: https://codereview.chromium.org/1433123002/#ps80001 (title: "REALLY answered Nat.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433123002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
