Skip to content

Updates to use node24#66

Merged
ethomson merged 1 commit into
test-summary:mainfrom
CharlieM312:feature/node-24-update
May 7, 2026
Merged

Updates to use node24#66
ethomson merged 1 commit into
test-summary:mainfrom
CharlieM312:feature/node-24-update

Conversation

@CharlieM312
Copy link
Copy Markdown
Contributor

@CharlieM312 CharlieM312 commented Mar 14, 2026 •
edited
Loading

Updates to change the action to use node24
Also fixed ESLint

@CharlieM312
Copy link
Copy Markdown
Contributor Author

CharlieM312 commented Mar 14, 2026

@ethomson

@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch 3 times, most recently from 11f1bd9 to f90fc99 Compare March 16, 2026 11:07
@ethomson
Copy link
Copy Markdown
Member

ethomson commented Mar 16, 2026

Thanks for the change! This PR is pretty large — 2560 additions & 4324 deletions — and a lot of that looks like formatting changes. 🥴

It's hard to review a large diff, especially when a lot of it is just noisy formatting changes that are noops. Can you not include formatting changes with the functional changes so that this is easier to review? If you think the formatting changes are useful, having each in a separate commit (so that they can be reviewed separately) would be grand. If they're not useful then let's not even make them in the first place. 🙏

@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch from f90fc99 to 12c1c46 Compare March 16, 2026 22:14
@CharlieM312
Copy link
Copy Markdown
Contributor Author

CharlieM312 commented Mar 16, 2026

Hi @ethomson, I've removed all formatting changes and just kept the functional ones, PR should be a lot smaller now

@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch from 12c1c46 to 5966676 Compare March 16, 2026 22:27
CharlieM312
CharlieM312 commented Mar 16, 2026
View reviewed changes
Comment thread src/index.ts
}

let show = TestStatus.Fail
let show: number = TestStatus.Fail
Copy link
Copy Markdown
Contributor Author

@CharlieM312 CharlieM312 Mar 16, 2026

Choose a reason for hiding this comment

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

show is being directly compared to a number below hence me adding this

CharlieM312
CharlieM312 commented Mar 16, 2026
View reviewed changes
Comment thread test/junit.ts
const normalizeDetails = (value: string | undefined) =>
value?.replace(/\r\n/g, "\n").trimEnd()

expect(normalizeDetails(case_with_message.details)).to.eql(
Copy link
Copy Markdown
Contributor Author

@CharlieM312 CharlieM312 Mar 16, 2026

Choose a reason for hiding this comment

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

Fixes a failing test due to just line spacing/trim issues it seems

CharlieM312
CharlieM312 commented Mar 16, 2026
View reviewed changes
Comment thread src/index.ts
import * as util from "util"
import * as core from "@actions/core"
import * as glob from "glob-promise"
import * as glob from "glob"
Copy link
Copy Markdown
Contributor Author

@CharlieM312 CharlieM312 Mar 16, 2026

Choose a reason for hiding this comment

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

glob-promise is now part of glob so updated it and changed the package around

@sergio-nsk
Copy link
Copy Markdown

sergio-nsk commented Mar 17, 2026

It's hard to review a large diff, especially when a lot of it is just noisy formatting changes that are noops

You can hide whitespace changes.
image

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Mar 17, 2026

Hi @ethomson, I've removed all formatting changes and just kept the functional ones, PR should be a lot smaller now

Thank you @CharlieM312 ! 🙏 I appreciate it.

@CharlieM312
Copy link
Copy Markdown
Contributor Author

CharlieM312 commented Mar 24, 2026

@ethomson - any plans to review and merge this PR? Happy to make changes if needed

@CharlieM312 CharlieM312 mentioned this pull request Apr 10, 2026
Open
@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch from 5966676 to 19544b1 Compare April 12, 2026 15:23
This was referenced Apr 20, 2026
Closed
Closed
@jagthedrummer
Copy link
Copy Markdown
Contributor

jagthedrummer commented May 4, 2026

Would love to see this or #70 or something get merged and released. Is there anything any of us can do to help?

@jagthedrummer
Copy link
Copy Markdown
Contributor

jagthedrummer commented May 4, 2026

Just for the record I added the suggested ENV var FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true to my workflow and it ran just fine. I think both this PR and #70 do way more than necessary to address the node 20 deprecation issue. (Not to say that the extra things are bad, just that there may be a simpler avenue available to address the deprecation.)

@jagthedrummer jagthedrummer mentioned this pull request May 4, 2026
Merged
@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch from 19544b1 to 45e9f04 Compare May 5, 2026 10:47
@CharlieM312
Copy link
Copy Markdown
Contributor Author

CharlieM312 commented May 5, 2026

Just for the record I added the suggested ENV var FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true to my workflow and it ran just fine. I think both this PR and #70 do way more than necessary to address the node 20 deprecation issue. (Not to say that the extra things are bad, just that there may be a simpler avenue available to address the deprecation.)

true - it would be good to get one PR merged

@CharlieM312 CharlieM312 force-pushed the feature/node-24-update branch from 45e9f04 to a36cf3b Compare May 5, 2026 13:23
@jagthedrummer
Copy link
Copy Markdown
Contributor

jagthedrummer commented May 5, 2026

I opened #71 with the smallest possible change I could make to address only the deprecation, in case that would make things easier for @ethomson and crew.

@ethomson ethomson merged commit 8f59535 into test-summary:main May 7, 2026
@CharlieM312 CharlieM312 deleted the feature/node-24-update branch May 7, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CharlieM312 @ethomson @sergio-nsk @jagthedrummer