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

Upload npm logs during debug or failure runs #61289

Open
wants to merge 20 commits into
base: trunk
Choose a base branch
from
Open

Upload npm logs during debug or failure runs #61289

wants to merge 20 commits into from

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented May 1, 2024 •
edited
Loading

What?

This rolls back the most recent update to the actions/setup-node GitHub Action.
This adds steps to upload npm logs as an artifact when running in debug mode or a failure is encountered.

Why?

When using a Windows based runner, there are some failures occurring when the npm ci script is run.

See this workflow run associated with #61138.

How?

This downgrades the actions/setup-node version from the latest (4.0.2) to 4.0.1, which fixes the problem in my testing (see #61288). It's not clear what's causing the problem, but it's likely related to the Windows related changes in 4.0.2 to add support for arm64.

@desrosj desrosj requested a review from DaniGuardiola May 1, 2024 15:45
@desrosj desrosj self-assigned this May 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 1, 2024 •
edited
Loading

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: desrosj <[email protected]>
Co-authored-by: DaniGuardiola <[email protected]>
Co-authored-by: sirreal <[email protected]>
Co-authored-by: kevin940726 <[email protected]>

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

@desrosj desrosj added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label May 1, 2024
This was referenced May 1, 2024
Merged
Merged
@desrosj
Copy link
Contributor Author

desrosj commented May 1, 2024

Hmm, on second glance, this may not be fixing the issue. Reverting to draft to continue investigating.

@desrosj desrosj marked this pull request as draft May 1, 2024 16:33
@github-actions GitHub Actions
Copy link

github-actions bot commented May 1, 2024 •
edited
Loading

Flaky tests detected in f21b053.
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/8913631643
📝 Reported issues:

@desrosj
Copy link
Contributor Author

desrosj commented May 1, 2024

I'm honestly not sure what's going on now. This PR fully reverts #61211 now, and the issue is still present.

@kevin940726 do you have any ideas by chance? @DaniGuardiola did you find anything in your debugging on the other PR?

@DaniGuardiola
Copy link
Contributor

I'm in the process of debugging it, I'll report back when I either find something conclusive or... give up :)

@DaniGuardiola
Copy link
Contributor

Feel free to fully take over this and I will stop debugging on my branch. Seems like you were already able to get some useful insights, according to #61138 (comment)

I was at a meetup so couldn't spend enough time here. Thank you for taking on this!

@desrosj
Avoid using matrix values.
0cdc4f2 
The available matrix options are not consistent across workflows.
DaniGuardiola
DaniGuardiola reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@DaniGuardiola DaniGuardiola left a comment

Choose a reason for hiding this comment

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

Looks good so far. Did you manage to find a consistent solution or workaround? If not, let's merge this as is and rename it just to get the nicer logs, and we can leave a task open to debug further. The action is not marked as required for now so it won't be blocking: https://wordpress.slack.com/archives/C02QB2JS7/p1714721524597469?thread_ts=1714590550.533979&cid=C02QB2JS7

@DaniGuardiola
Copy link
Contributor

Heads up, I removed the logging stuff from my PR but somehow managed to not push it and it got merged 😅

https://github.com/WordPress/gutenberg/pull/61138/files#diff-bd14ee39f26ac8f8790a63a11c34e9e0f8ee8fe1e08f1d425c1874bc2f56e973

You should be able to simply override the trunk version with yours in this PR.

@desrosj desrosj requested a review from DaniGuardiola May 14, 2024 14:22
@desrosj
Copy link
Contributor Author

desrosj commented May 14, 2024

I've gone and merged the changes from trunk and updated the PR. Should be good for a review now!

@desrosj desrosj marked this pull request as ready for review May 14, 2024 14:23
@desrosj desrosj requested review from gziolo and sirreal May 14, 2024 14:23
sirreal
sirreal approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Let's try it, those failure are very annoying 🙂

@desrosj desrosj changed the title Rollback most recent setup-node update Upload npm logs during debug or failure runs May 14, 2024
@desrosj
Copy link
Contributor Author

desrosj commented May 14, 2024

Sorry, I forgot to change the PR title and brief description to reflect the new intent of this PR. @sirreal I think we've filtered out the failures elsewhere, but this just gives better log preservation for debug purposes.

@sirreal sirreal self-requested a review May 15, 2024 15:42
@kevin940726
Copy link
Member

I suspect the recent failures might be related to the node_modules cache we introduced in #45932, but I don't have any theory backing the suspicion 😅. We can also try disabling caching node_modules on Windows and see if that fixes the issue.

DaniGuardiola
DaniGuardiola requested changes May 22, 2024
View reviewed changes
Copy link
Contributor

@DaniGuardiola DaniGuardiola left a comment

Choose a reason for hiding this comment

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

Looks great, but we need to remove this thing I accidentally introduced in a different PR.

.github/setup-node/action.yml Outdated
@@ -42,6 +42,19 @@ runs:
name: npm-logs
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove this task.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref: #61289 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DaniGuardiola Could you have another look? I've merged trunk into this PR and this should now be addressed.

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

@DaniGuardiola DaniGuardiola DaniGuardiola requested changes

@gziolo gziolo Awaiting requested review from gziolo

@sirreal sirreal Awaiting requested review from sirreal

Requested changes must be addressed to merge this pull request.

Assignees

@desrosj desrosj

Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@desrosj @DaniGuardiola @kevin940726 @sirreal