FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix tests when Amaro is unavailable by richardlau · Pull Request #55320 · nodejs/node · GitHub

/ node Public

test: fix tests when Amaro is unavailable - #55320

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:withoutamaro
Oct 11, 2024
ConversationCommits (1)ChecksFiles changed
Merged

test: fix tests when Amaro is unavailable#55320
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
richardlau:withoutamaro

Conversation

richardlau commented Oct 8, 2024

Copy link
Copy Markdown
Member

Fix two tests that fail when node is configured --without-amaro.

nodejs-github-bot commented Oct 8, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Oct 8, 2024
richardlau marked this pull request as draft October 8, 2024 15:03

avivkeller commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This isn't right, process.features.typescript will check whether the currently running process supports typescript, not whether the executable does. The correct value to check would be process.config.variables.node_use_amaro

avivkeller added the strip-types Issues or PRs related to strip-types support label Oct 8, 2024

codecov Bot commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (9f9069d) to head (207741a).
Report is 248 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55320      +/-   ##
==========================================
- Coverage   88.39%   88.39%   -0.01%     
==========================================
  Files         652      652              
  Lines      186777   186777              
  Branches    36039    36039              
==========================================
- Hits       165109   165100       -9     
- Misses      14917    14927      +10     
+ Partials     6751     6750       -1

see 30 files with indirect coverage changes

aduh95 commented Oct 8, 2024

Copy link
Copy Markdown
Contributor

We might want fix the commit message as well s/when amaro unavailable/when Amaro is unavailable/.

richardlau commented Oct 8, 2024

Copy link
Copy Markdown
Member Author

This isn't right, process.features.typescript will check whether the currently running process supports typescript, not whether the executable does. The correct value to check would be process.config.variables.node_use_amaro

I figured this out (which is why I changed the state back to draft). Currently running tests locally (both with and without amaro) before pushing up the correction.

This comment was marked as off-topic.

Sign in to view

marco-ippolito commented Oct 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

also should actually set process.features.typescript to false if amaro is not loaded

richardlau marked this pull request as ready for review October 8, 2024 15:58
richardlau changed the title test: fix tests when amaro unavailable test: fix tests when Amaro is unavailable Oct 8, 2024
aduh95 mentioned this pull request Oct 8, 2024
Merged
richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 8, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 8, 2024

This comment was marked as outdated.

Sign in to view
avivkeller added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 8, 2024
aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 9, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 10, 2024

This comment was marked as outdated.

Sign in to view
nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 10, 2024
richardlau added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 10, 2024
nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 10, 2024

This comment was marked as outdated.

Sign in to view
aduh95 removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 10, 2024

aduh95 commented Oct 10, 2024

Copy link
Copy Markdown
Contributor

Can you rebase?

richardlau commented Oct 10, 2024

Copy link
Copy Markdown
Member Author

Can you rebase?

👍. Will do so once the linter failure on main is fixed.

test: fix tests when Amaro is unavailable
207741a 
Fix two tests that fail when `node` is configured `--without-amaro`.
richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2024

richardlau commented Oct 11, 2024

Copy link
Copy Markdown
Member Author

Can you rebase?

👍. Will do so once the linter failure on main is fixed.

Rebased.

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2024

nodejs-github-bot commented Oct 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63040/

nodejs-github-bot commented Oct 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63041/

marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2024
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2024
nodejs-github-bot merged commit 82dab76 into nodejs:main Oct 11, 2024

nodejs-github-bot commented Oct 11, 2024

Copy link
Copy Markdown
Collaborator

Landed in 82dab76

aduh95 pushed a commit that referenced this pull request Oct 11, 2024
test: fix tests when Amaro is unavailable
3273707 
Fix two tests that fail when `node` is configured `--without-amaro`.

PR-URL: #55320
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
aduh95 mentioned this pull request Oct 11, 2024
Merged
richardlau deleted the withoutamaro branch October 11, 2024 16:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca approved these changes
aduh95 aduh95 approved these changes
marco-ippolito marco-ippolito approved these changes
atlowChemi atlowChemi approved these changes

Assignees

No one assigned

Labels

esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL