Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

/css/css-transitions/* are disabled/flaky in mozilla chromium #11046

Open
zcorpan opened this issue May 17, 2018 · 22 comments · May be fixed by #11057
Open

/css/css-transitions/* are disabled/flaky in mozilla chromium #11046

zcorpan opened this issue May 17, 2018 · 22 comments · May be fixed by #11057
Labels
flaky

Comments

@zcorpan
Copy link
Member

zcorpan commented May 17, 2018 •
edited

http://bocoup.github.io/wpt-disabled-tests-report/

Investigate what's up with these tests:

Path Products Results Bugs
/css/css-transitions/transitions-animatable-properties-01.html mozilla chromium disabled [ Timeout Failure ] https://bugzilla.mozilla.org/show_bug.cgi?id=1356224 https://crbug.com/626703
/css/css-transitions-1/__dir__ mozilla disabled https://bugzilla.mozilla.org/show_bug.cgi?id=1356627
/css/css-transitions/properties-value-001.html mozilla disabled: None
/css/css-transitions/properties-value-inherit-001.html mozilla disabled: None
/css/css-transitions/properties-value-inherit-002.html mozilla disabled: None
/css/css-transitions/properties-value-inherit-003.html mozilla disabled: None
/css/css-transitions/transition-property-017.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-030.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-015.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-021.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-039.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-008.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-002.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-012.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-014.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-016.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-007.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-009.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-029.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-023.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-027.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-010.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-003.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-011.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-008.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-011.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-020.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-013.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-012.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-031.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-delay-000.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-041.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-026.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-duration-003.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-delay-002.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-005.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-004.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-006.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-018.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-022.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-duration-002.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-024.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-009.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-037.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-007.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-005.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-042.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-036.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-038.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-delay-003.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-010.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-028.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-004.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-043.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-040.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-019.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-034.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-045.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-035.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-033.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-003.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-044.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-timing-function-006.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-duration-004.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-032.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-025.html chromium [ Skip ] https://crbug.com/626703
/css/css-transitions/transition-property-013.html chromium [ Skip ] https://crbug.com/626703

cc @csnardi @ZhuoyuQian @cvrebert
@zcorpan zcorpan added the flaky label May 17, 2018
@zcorpan zcorpan self-assigned this May 17, 2018
@csnardi
Copy link
Member

csnardi commented May 17, 2018

Most of the tests disabled in Chromium are because they are manual tests, and thus can't be automatically run by CI.

@zcorpan
Copy link
Member Author

zcorpan commented May 17, 2018 •
edited by jugglinmike

css/css-transitions/transitions-animatable-properties-01.html should have <meta name="timeout" content="long"> and maybe split up with variants

The manual tests should have -manual in the file name.

zcorpan added a commit that referenced this issue May 17, 2018
@zcorpan
[css-transitions] Split up with variant, set long timeout, rename m…
0bf1b10 
…anual tests

* Split up transitions-animatable-properties-01.html with `variant` and set long timeout.
* Set long timeout for properties-value-*.
* Rename many manual tests to have -manual suffix.

Fixes #11046.
@zcorpan zcorpan linked a pull request May 17, 2018 that will close this issue
Open
@jugglinmike
Copy link
Contributor

@zcorpan

The manual tests should have -manual in the file name.

I suggested something similar elsewhere, but @gsnedders discouraged such changes:

We have thousands of tests in css/ that don't follow the larger convention; we deliberately added support for <meta name=flags> when merging the test suites.

@zcorpan
Copy link
Member Author

zcorpan commented May 19, 2018

I think we should be moving towards a single convention for wpt. Having special rules in css/ makes it harder for newcomers to contribute.

@jugglinmike
Copy link
Contributor

I agree, though this would be kind of disruptive:

$ git rev-parse HEAD
11a87acba01204660f199976158c36bc9758c37c
$ git grep -lE 'meta.*flags' . | grep -v '.manual' | wc -l
16078

Consumers probably have many of the inaccurate file names in their test blacklists.

@gsnedders Am I right in thinking that the reason this hasn't been done is to limit churn? If so, would it be any more palatable if we provided folks with a Python script that could find and replace the affected file names in arbitrary text files?

@zcorpan
Copy link
Member Author

zcorpan commented May 21, 2018

flags is not only for manual tests

@jugglinmike
Copy link
Contributor

Could it be that this query is actually what we want?

$ git grep -lE 'meta.*flags.*\binteract\b' css | grep -v '.manual' | wc -l
770

@gsnedders
Copy link
Member

gsnedders commented May 22, 2018 •
edited

@jugglinmike No, we want something more general.

Commenting out from lint.whitelist:

CONTENT-VISUAL: css/*
CONTENT-MANUAL: css/*

(i.e., both those that should be -visual and -manual)

and running the lint:

There were 4532 errors (CONTENT-VISUAL: 3172 CONTENT-MANUAL: 1360)

@gsnedders
Copy link
Member

Also, FWIW, I believe there are some implementations that rely on the "print" flag to mark tests as print tests that can be automated, though we don't have anything for that currently in WPT.

@gsnedders
Copy link
Member

gsnedders commented May 22, 2018 •
edited by zcorpan

Looking into sourcefile.py, to be manual you must have one of the following flags: "animated", "font", "history", "interact", "paged", "speech", "userstyle" (not quite sure why history is there, especially given that is essentially unused). To be a CSS visual test, you merely need to have a <link rel=help> and not be in an ignored/reference directory.

@zcorpan
Copy link
Member Author

zcorpan commented May 22, 2018

OK, so 1360 tests would need renaming to cover manual tests. I think that's worthwhile to do. @jgraham @foolip any opinion?

@foolip
Copy link
Member

foolip commented May 22, 2018

Are these tests which are now misclassified, or do you mean getting rid of flags and only using the filename to determine what is a manual test?

@gsnedders
Copy link
Member

@foolip getting rid of the flags and renaming (I'm less convinced of the value of removing the metadata); I think we'd need to add something in way of support for finding print (i.e., paged media) tests before we do it because a lot of tests would get marked as manual just because we don't currently support print tests, given otherwise we'll remove the metadata that allows one to find print tests making it very hard to readd in the future.

@zcorpan if we're doing this for -manual, we should do it for -visual too IMO.

@gsnedders
Copy link
Member

Let's keep the discussion about -manual in #5381, actually.

@zcorpan zcorpan assigned gsnedders and unassigned zcorpan Jun 21, 2018
@gsnedders
Copy link
Member

gsnedders commented Jun 28, 2018 •
edited

From @birtles in the Mozilla bug:

(I wish people would resist the urge to go and write their own complicated test harness. It always leads to spending more time debugging the test harness than the code under test. This one even does its own vendor prefixing and timeout handling :/ )

Looking at assertExpectedEventsFunc in generalParallelTest.js, it doesn't actually test if the transitionend event has been received or not. If it hasn't been received then the string it will compare will be an empty string, hence the failure.

So I'd say we're simply failing to wait long enough for the transitionend event here.

It's not worth spot-fixing this, however. This test, and I suspect many others in this folder, should be rewritten to use testharness.js and generally be a lot simpler. This test shouldn't even be testing events in the first place.

I'm quite ok with all these tests being disabled until we rewrite them.

On the whole, I agree that the problem with the automated test is basically the entire test harness.

I don't know if there are Gecko or Blink tests that could be upstreamed to replace them?

@birtles
Copy link
Contributor

birtles commented Jun 29, 2018

I did fix a bunch of tests here last year some time, and was working on fixing the rest in this branch:

https://github.com/birtles/web-platform-tests/commits/update-more-transition-event-tests

I've yet to finish, however.

@gsnedders
Copy link
Member

@birtles if some have been fixed, should some get re-enabled in Gecko?

@gsnedders
Copy link
Member

Opened https://bugzilla.mozilla.org/show_bug.cgi?id=1472172 on the Gecko side for all the tests using the old harness:

before-DOMContentLoaded-001.html
before-load-001.html
changing-while-transition.html
detached-container-001.html
hidden-container-001.html
properties-value-001.html
properties-value-002.html
properties-value-003.html
properties-value-auto-001.html
properties-value-implicit-001.html
properties-value-inherit-001.html
properties-value-inherit-002.html
properties-value-inherit-003.html
pseudo-elements-001.html

@birtles
Copy link
Contributor

birtles commented Jul 1, 2018

Thanks Geoffrey!

@birtles
Copy link
Contributor

birtles commented Jul 5, 2018

I did fix a bunch of tests here last year some time, and was working on fixing the rest in this branch:

https://github.com/birtles/web-platform-tests/commits/update-more-transition-event-tests

I've yet to finish, however.

Sorry, wrong branch (the one above has already been merged). This is the one where I'm updating these tests:

https://github.com/birtles/web-platform-tests/commits/fix-transition-tests

@gsnedders
Copy link
Member

@birtles we should land what you've already fixed, no?

@gsnedders gsnedders removed their assignment Apr 1, 2019
@birtles
Copy link
Contributor

birtles commented Apr 2, 2019

I should finish fixing a few more tests first I guess. Let me bring it up at our Web Animations telcon tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky
Projects
None yet
Milestone
No milestone
6 participants
@gsnedders @zcorpan @foolip @jugglinmike @birtles @csnardi