Skip to content

tools: support environment variables via comments - #58186

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
pmarchini:feat/py_test_runner/support_env_notation
May 11, 2025
Merged

tools: support environment variables via comments#58186
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
pmarchini:feat/py_test_runner/support_env_notation

Conversation

@pmarchini

Copy link
Copy Markdown
Member

It should address #58179 by adding support for environment variables via comments.
I've not run the entire test suite, only a subset.
A full CI run will be needed to catch any potential regressions!

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels May 5, 2025

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

JS portion LGTM.

@pmarchini

Copy link
Copy Markdown
MemberAuthor

I've already seen a bunch of regressions, changing the method signature may not have been the best decision(I'm trying to avoid reading the test file twice)
I'll have another look ASAP!

@pmarchini
pmarchiniforce-pushed the feat/py_test_runner/support_env_notation branch from 830bb71 to d674f42CompareMay 5, 2025 22:29
@codecov

codecovBot commented May 5, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.17%. Comparing base (4acb854) to head (dfeec6e).
Report is 297 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #58186 +/- ##
==========================================
+ Coverage 90.15% 90.17% +0.02% 
==========================================
Files 630 629 -1 Lines 186756 186643 -113 Branches 36648 36651 +3 ==========================================
- Hits 168362 168306 -56 + Misses 11193 11133 -60 - Partials 7201 7204 +3 

see 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pmarchini
pmarchiniforce-pushed the feat/py_test_runner/support_env_notation branch from d674f42 to 77e4612CompareMay 6, 2025 07:48

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a nit but I'd prefer the more compact: // Env: ...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

+1 (@joyeecheung suggested the same name)
I’ll go ahead and update the PR

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done!

@pmarchini
pmarchiniforce-pushed the feat/py_test_runner/support_env_notation branch from 77e4612 to dfeec6eCompareMay 8, 2025 15:37
@pmarchinipmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 9, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@pmarchinipmarchini added the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 6184730 into nodejs:mainMay 11, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 6184730

targos pushed a commit that referenced this pull request May 16, 2025
PR-URL: #58186
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@aduh95aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jun 10, 2025
@aduh95

Copy link
Copy Markdown
Contributor

Added tests are failing when cherry-picked on v22.x-staging, it will require a manual backport if we want it there

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

Labels

backport-requested-v22.xPRs awaiting manual backport to the v22.x-staging branch.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@pmarchini@nodejs-github-bot@aduh95@jasnell@cjihrig@marco-ippolito