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

Allow hiding test state for first-contentful-paint tests #24170

Merged
merged 3 commits into from
Jul 7, 2020
Merged

Allow hiding test state for first-contentful-paint tests #24170

merged 3 commits into from
Jul 7, 2020

Conversation

sefeng211
Copy link
Contributor

@sefeng211 sefeng211 commented Jun 16, 2020 •
edited
Loading

The testharness' show_status function call interfere
the tests result because it would display a text to the document
while the tests are running.

The issue is this text could be considered as the first-contentful-paint,
which is not what we expect.

We fix it by adding a new setup property to not displaying this text.

This PR intends to fix #24089

@sefeng211
Copy link
Contributor Author

sefeng211 commented Jun 16, 2020 •
edited
Loading

cc @noamr @npm1

npm1
npm1 reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

paint-timing change looks fine to me, although perhaps we want to apply this to all paint-timing tests and not just those using test_fcp?

@sefeng211
Copy link
Contributor Author

@npm1 Agreed.

PR updated.

npm1
npm1 reviewed Jun 18, 2020
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

Thanks! I'm fine with landing this % comment, but have someone from infra take a look?

resources/testharness.js Outdated
@@ -2510,6 +2510,7 @@ policies and contribution forms [3].
this.test_done_callbacks = [];
this.all_done_callbacks = [];

this.do_notify_test_state = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry I missed this before you added more but it probably makes sense to make this hide_test_state = false since we generally want the default value for booleans to be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. Fixed

@sefeng211 sefeng211 requested a review from npm1 June 18, 2020 17:18
npm1
npm1 approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@npm1 npm1 left a comment

Choose a reason for hiding this comment

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

paint-timing LGTM

@npm1 npm1 requested a review from Hexcles June 22, 2020 20:22
@Hexcles
Copy link
Member

Hexcles commented Jun 22, 2020

This seems very reasonable to me, but I'm not sure if we should do an RFC @web-platform-tests/wpt-core-team .

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 23, 2020

We probably should do an rfc, yeah

@sefeng211
Copy link
Contributor Author

Not sure what an rfc is, is there any action that I should take?

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 23, 2020

See https://github.com/web-platform-tests/rfcs

@sefeng211 sefeng211 mentioned this pull request Jun 23, 2020
Merged
@sefeng211
Allow hiding test state in testharness
a0159a3 
Some tests are sensitive to the content of the document, thus
show_status may interfere the test results.

We add a new setup property to allow hiding the test state output.
@sefeng211
Add hide_test_state property to the document
bdf74d3
@sefeng211
Hide test state for all paint timing tests
c999557
@sefeng211 sefeng211 changed the title Disable notifying test state for first-contentful-paint tests Allow hiding test state for first-contentful-paint tests Jul 6, 2020
@sefeng211
Copy link
Contributor Author

The RFC is merged.

I also updated the patch to follow @stephenmcgruer's suggestion, which makes sense because show_status only outputs the test state while tests are running.

@sefeng211 sefeng211 requested a review from npm1 July 6, 2020 17:05
Hexcles
Hexcles approved these changes Jul 6, 2020
View reviewed changes
Copy link
Member

@Hexcles Hexcles left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for following through the process, Sean!

@stephenmcgruer
Copy link
Contributor

Failed chrome-dev-stability:

4:15.46 INFO ## Unstable results ##
 4:15.46 INFO |                    Test                    |                  Subtest                   |          Results           |                                     Messages                                    |
 4:15.46 INFO |--------------------------------------------|--------------------------------------------|----------------------------|---------------------------------------------------------------------------------|
 4:15.46 INFO | `/paint-timing/fcp-only/fcp-gradient.html` | `Gradients should not count as contentful` | **FAIL: 9/10, PASS: 1/10** | `assert_equals: First contentful paint marked too early.  expected 0 but got 1` |
 4:15.46 INFO

From wpt.fyi history, looks like a previously existing flake but I'm a little interested here just because the goal of this PR was to remove unexpectedly-early FCPs (due to test infra involvement).

@sefeng211 - I leave this up to you; should this PR be admin-merged, or is this flake something that is worth pausing to investigate?

@npm1
Copy link
Contributor

npm1 commented Jul 7, 2020

I think the flake there is caused by Chrome's failure to implement FCP properly for that particular testcase. If it's flaky it probably means that the timeout may not be long enough to guarantee that the incorrectly early FCP is always fired, but this is unrelated to this PR.

@stephenmcgruer
Copy link
Contributor

Ack, admin-merging. :)

@stephenmcgruer stephenmcgruer merged commit ec9f61d into web-platform-tests:master Jul 7, 2020
@Hexcles
Copy link
Member

Hexcles commented Jul 7, 2020

If this is a Chromium bug, it'd be great to file an issue on crbug.

@npm1
Copy link
Contributor

npm1 commented Jul 7, 2020

There's already https://bugs.chromium.org/p/chromium/issues/detail?id=1062984 (apparently flakiness is not fixed though?) and https://bugs.chromium.org/p/chromium/issues/detail?id=1071450

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

@Hexcles Hexcles Hexcles approved these changes

@jgraham jgraham Awaiting requested review from jgraham

@spanicker spanicker Awaiting requested review from spanicker

@sideshowbarker sideshowbarker Awaiting requested review from sideshowbarker

@npm1 npm1 Awaiting requested review from npm1

Assignees

@jgraham jgraham

Labels
docs infra paint-timing testharness.js wg-webperf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

testharness itself may interfere the result of first contentful paint tests?
7 participants
@sefeng211 @Hexcles @Ms2ger @stephenmcgruer @npm1 @jgraham @wpt-pr-bot