Skip to content

Conversation

@paulirish
Copy link
Member

@paulirish paulirish commented Dec 9, 2020

Example failure:
image

Most of the smoke_3_ToT failures ive been seeing this afternoon are coming from this assertion.


The animation reliably gets kUnsupportedCSSProperty (1 << 13) === 8192. (which is what we want)

Sometimes it also gets kTargetHasInvalidCompositingState (1 << 5) == 32. (but not always) Together they sum to 8224.

@adamraine and I have investigated kTargetHasInvalidCompositingState a few times and it's not actionable, in part because it shows up under very hard-to-predict circumstances.. Basically its presence is flaky

so this PR changes the smoketest expectation to handle it being there and not.


on the impl: i stated with /(8192)|(8224)/ but our report-assert doesn't expect to apply regexes to numbers, and I didn't want to deal with that. Overall i'm happier with this anyway.

@paulirish paulirish requested a review from a team as a code owner December 9, 2020 01:48
@paulirish paulirish requested review from connorjclark and removed request for a team December 9, 2020 01:48
@google-cla google-cla bot added the cla: yes label Dec 9, 2020
@paulirish paulirish added the P1 label Dec 9, 2020
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was gonna ask about the regex because it feels like the more straightforward fix, but I suppose it is weird to coerce numbers.

LGTM nice find!

@adamraine
Copy link
Contributor

Thanks for taking care of this!

Might be worth investigating why the test became flaky all of a sudden.

@paulirish
Copy link
Member Author

Might be worth investigating why the test became flaky all of a sudden.

yah. it only happens on ToT and not stable, so presumably its a commit that landed in the past day or so.

@paulirish paulirish merged commit 0e8e01d into master Dec 9, 2020
@paulirish paulirish deleted the failuremaskflake branch December 9, 2020 04:16
@adamraine
Copy link
Contributor

Found the culprit: https://chromium.googlesource.com/chromium/src/+/f0946b3ebc528e5eb4a1a9c033357d1df7a436b6

Looks like background-color animations will soon be supported on the compositor. Maybe we should update the smoke test to use a different CSS property so we aren't caught off guard by this test failing again.

@paulirish
Copy link
Member Author

@adamraine nice find! did you bisect or just realize this landed at the right time to explain things?

Maybe we should update the smoke test to use a different CSS property so we aren't caught off guard by this test failing again.

sgtm! yah i can see it happening again. (partially thanks to you! 😝 )

@adamraine
Copy link
Contributor

did you bisect or just realize this landed at the right time to explain things?

Bisect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants