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

CI: Add build-module to unit test assets #65499

Closed
wants to merge 2 commits into from

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Sep 19, 2024

What?

build-module was not included in the built unit test assets which can cause test failures.

This was overlooked in #65064.

This is a fix extracted from #65460.

Why?

The compiled assets are cached and shared across unit tests. This applies to both build and build-module directories.

Testing Instructions

CI passes. Also review CI on #65460.

@sirreal sirreal added the [Type] Build Tooling Issues or PRs related to build tooling label Sep 19, 2024
@sirreal sirreal force-pushed the fix/build-module-cached-unit-test-asset branch 2 times, most recently from e301514 to a9a3825 Compare September 19, 2024 20:41
Copy link

Flaky tests detected in a9a3825.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10948606263
📝 Reported issues:

@sirreal sirreal force-pushed the fix/build-module-cached-unit-test-asset branch from a9a3825 to 487a982 Compare September 20, 2024 09:09
@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

Why do I see these test failures here?

19) WP_Block_Supports_Colors_Test::test_gradient_with_individual_skipped_serialization_block_supports
Error: Call to undefined function gutenberg_style_engine_get_styles()

@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

Ok, this has fixed things. You can see PHP tests passing in #65460 now (with this merged).

@sirreal sirreal marked this pull request as ready for review September 20, 2024 10:23
@sirreal sirreal requested a review from desrosj as a code owner September 20, 2024 10:23
@sirreal sirreal requested a review from gziolo September 20, 2024 10:23
Copy link

github-actions bot commented Sep 20, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: sirreal <[email protected]>
Co-authored-by: gziolo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@sirreal sirreal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2024
@sirreal
Copy link
Member Author

sirreal commented Sep 20, 2024

I've aded the backport label. This won't affect the plugin at all but it could cause problems on CI on branches if this is not fixed.

Comment on lines +138 to +140
path: |
./build/
./build-module/
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we need to list all the paths.

If multiple paths are provided as input, the least common ancestor of all the search paths will be used as the root directory of the artifact. Exclude paths do not affect the directory structure.

@@ -212,7 +214,6 @@ jobs:
uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4.1.8
with:
name: build-assets
path: ./build
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we remove the path so that it extracts all the assets correctly instead of naming them.

Before, since there was one directory in the aftifact, naming it build was OK. Now that we have a parent directory with build and build-module it just goes into the working directory.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Tested with the linked PR and the changes make sense 👍🏻

@gziolo gziolo deleted the fix/build-module-cached-unit-test-asset branch September 20, 2024 12:42
@sirreal sirreal removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants