Skip to content

fix: fix npx running in test, make approve-scripts test less flaky - #31501

Merged
nathanwhit merged 3 commits into
denoland:mainfrom
nathanwhit:test-npx-bad
Dec 5, 2025
Merged

nathanwhit merged 3 commits into
denoland:mainfrom
nathanwhit:test-npx-bad

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

It was doing npx cowsay but cowsay wasn't listed as a dependency, so it was falling back to actual npx.

I also found that the cowsay part sometimes overlaps with the progress bar, so checking for 'install' is more reliable

@coderabbitai

coderabbitai Bot commented Dec 4, 2025 •

Copy link
Copy Markdown

Walkthrough

This pull request adjusts test expectations and test fixtures. Two assertions in tests/integration/pm_tests.rs were changed to shorten the asserted lifecycle message from the exact text containing : running 'postinstall' script to a more generic : running. The test registry package.json for @denotest/node-lifecycle-scripts@1.0.0 adds a new dependency "cowsay": "1.5.0".

Pre-merge checks and finishing touches

βœ… Passed checks (3 passed)
Check name Status Explanation
Title check βœ… Passed The title accurately describes the main changes: fixing npx behavior in tests and reducing test flakiness by adjusting the assertion.
Description check βœ… Passed The description clearly explains why the changes were made: cowsay wasn't a dependency so tests fell back to system npx, and cowsay output overlapped with progress bars.
Docstring Coverage βœ… Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

πŸ“œ Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 00a3afb and dcf72b2.

β›” Files ignored due to path filters (5)
  • tests/specs/install/install_entrypoint/lifecycle.out is excluded by !**/*.out
  • tests/specs/npm/lifecycle_scripts/argument/all_lifecycles_not_run.out is excluded by !**/*.out
  • tests/specs/npm/lifecycle_scripts/argument/all_lifecycles_not_run_global.out is excluded by !**/*.out
  • tests/specs/npm/lifecycle_scripts/argument/future_install_all_lifecycles_not_run.out is excluded by !**/*.out
  • tests/specs/npm/lifecycle_scripts/argument/only_warns_first1.out is excluded by !**/*.out
πŸ“’ Files selected for processing (1)
  • tests/integration/pm_tests.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/integration/pm_tests.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test release linux-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: build libs
  • GitHub Check: lint debug windows-x86_64
  • GitHub Check: lint debug macos-x86_64

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nathanwhit
nathanwhit enabled auto-merge (squash) December 4, 2025 22:48
@nathanwhit
nathanwhit merged commit dd0aa99 into denoland:main Dec 5, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants