Skip to content

watch: reload env file for --env-file-if-exists#56643

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JonasBa:jb/watch/env-exists
Jan 25, 2025
Merged

watch: reload env file for --env-file-if-exists#56643
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
JonasBa:jb/watch/env-exists

Conversation

@JonasBa

@JonasBa JonasBa commented Jan 17, 2025

Copy link
Copy Markdown
Contributor

While using watch mode with --env-file-if-exists, reload env correctly

ref: #49148 (comment)

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 17, 2025
@JonasBa JonasBa mentioned this pull request Jan 17, 2025
8 tasks
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2025
Comment thread test/sequential/test-watch-mode.mjs Outdated
@codecov

codecov Bot commented Jan 17, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (08eeddf) to head (9122c4b).
Report is 1057 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56643      +/-   ##
==========================================
- Coverage   89.21%   89.20%   -0.01%     
==========================================
  Files         662      662              
  Lines      191968   191968              
  Branches    36955    36949       -6     
==========================================
- Hits       171269   171253      -16     
- Misses      13535    13551      +16     
  Partials     7164     7164              

see 37 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.

@anonrig anonrig removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 18, 2025
@jasnell jasnell added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 19, 2025
@JonasBa
JonasBa force-pushed the jb/watch/env-exists branch from 6e3bc49 to 055d0ca Compare January 21, 2025 15:57
@JonasBa

JonasBa commented Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

@jasnell my knowledge of the test runner is not great here, but I believe the tests were failing here due to sync loop and inner async test definition, which may have caused the first test completion to delete the file that the second test was relying on.

I have split the two tests (duplicated code aside from --env cmd) and ran them manually to confirm they pass.

@github-actions github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jan 21, 2025
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/12894525741

@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Jan 21, 2025
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig

anonrig commented Jan 24, 2025

Copy link
Copy Markdown
Member

let's hope the flakiness is gone now...

@JonasBa

JonasBa commented Jan 24, 2025

Copy link
Copy Markdown
Contributor Author

One more and it'll work, I promise

@JonasBa
JonasBa force-pushed the jb/watch/env-exists branch from 055d0ca to 9122c4b Compare January 24, 2025 15:38
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 59b3a8b into nodejs:main Jan 25, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 59b3a8b

const kKillSignal = 'SIGTERM';
const kShouldFilterModules = getOptionValue('--watch-path').length === 0;
const kEnvFile = getOptionValue('--env-file');
const kEnvFile = getOptionValue('--env-file') || getOptionValue('--env-file-if-exists');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@JonasBa If I'm reading this right, this just makes it so that one file referenced in --env-file-if-exists would be watched only if there is no file provided with --env-file. But the bug we still have is that you wouldn't be able to specify both an --env-file and an --env-file-if-exists argument and have both be watched.

@marco-ippolito

Copy link
Copy Markdown
Member

This commit requires manual backport to land on v20.x

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants