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.

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

Add auto wpt option to workflow #29673

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add auto wpt option to workflow #29673

wants to merge 1 commit into from

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented Apr 27, 2023

Introduces auto wpt option in Linux workflow, which runs wpt test for layout-engine based on file changes.

So now we have 3 wpt options:

  • test unconditionally runs wpt tests
  • auto runs wpt tests only if there are changes in layout folders (does are determined by filters based on selected layout)
  • sync unconditionally runs wpt tests and sync them

In main workflow layout2020 is in test mode (run on every change), layout2013 is in auto mode (wpt test are run only if changes are present in layout2013 folders), other try-* builds would still be working as expected.

@sagudev sagudev marked this pull request as ready for review May 2, 2023 06:43
.github/workflows/linux.yml Outdated Show resolved Hide resolved
.github/workflows/linux.yml Outdated Show resolved Hide resolved
@mrobinson
Copy link
Member

Thank you for doing this work. We've talked this and we feel like since we are doing a lot more effort on Layout 2020, we would like to always run the WPT suite in the main workflow for Layout 2020. See #29746.

@sagudev
Copy link
Member Author

sagudev commented May 16, 2023

The only change that was needed was setting layout2013 to auto and layout2020 to test, if we still want to have any tests running for layout2013. I also updated PR description.

@mrobinson
Copy link
Member

Apologies, I was not super clear in my last comment. For now, we'd like to try running WPT for both layout 2013 and layout 2020 in the main workflow.

@sagudev
Copy link
Member Author

sagudev commented May 18, 2023

I will keep this PR open, so when it is decided not to run layout2013 wpt tests anymore, this can be merged, to keep layout2013 in shape until, it gets completely removed.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #29866) made this pull request unmergeable. Please resolve the merge conflicts.

@mrobinson
Copy link
Member

I've been thinking a bit more about this and it's a really tricky change. Here are some examples of why:

  • Changes to components/script and really any directory in components can affect WPT test results.
  • Changes to Python code in python can alter the way that the test harness runs and affect WPT results.
  • Changes to any of the third-party dependencies in third_party or Cargo.lock can affect layout, painting, anything.
  • Changes to any of the tests or metadata can affect whether the WPT tests pass.
  • Changes to the workflow files in .github can break WPT tests (as I have done on multiple occasion 😅)

I think the number directories that we can change without wanting to run WPT tests are actually quite small. I do not know if it's possible to detect this as Servo currently is. Perhaps we could skip certain platforms if we only modify platform-specific code...but I'm not certain even that is possible.

@sagudev
Copy link
Member Author

sagudev commented Aug 2, 2023

Auto option wasn't meant to be used on layout2020, it is meant to provide best-effort CI runs for layout2013, when we want to deprecate layout2013 even further, kicking it out of main CI, but not removing from tree.

@mrobinson
Copy link
Member

This would first require switching the main bulk of tests over to be run via layout 2020.

Is the idea that this would run as the main workflow or that people would use a try=auto option?

@sagudev
Copy link
Member Author

sagudev commented Aug 2, 2023

This was planned to be run as part of main workflow. try=wpt would still run layout2013 no matter what. Given the recent changes we could also make parameter for normal try build to force layout2013 wpt test.

@mrobinson
Copy link
Member

This was planned to be run as part of main workflow. try=wpt would still run layout2013 no matter what. Given the recent changes we could also make parameter for normal try build to force layout2013 wpt test.

I think we must always run all WPT tests when landing changes, but perhaps try could skip jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants