Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: Fix missing imports in script #49489

Merged
merged 4 commits into from
Sep 6, 2023
Merged

Conversation

OshriAsulin
Copy link
Contributor

@OshriAsulin OshriAsulin commented Sep 4, 2023

Fixes: #49488

The script was missing necessary imports for the 'run' function and the 'path' module, causing it to fail. This commit adds the missing imports and resolves the issue.

  • Import 'run' from the appropriate module.
  • Import 'path' to resolve file paths.

The script should now run without errors.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. test_runner Issues and PRs related to the test runner subsystem. labels Sep 4, 2023
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

There is also a CJS example on line 920 of the file that needs to be updated as well.

doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated
const { tap } = require('node:test/reporters');
const { tap, run } = require('node:test/reporters');
const process = require('process');
const path = require('path');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const path = require('path');
const path = require('node:path');

doc/api/test.md Outdated
import process from 'node:process';
import path from 'path';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import path from 'path';
import path from 'node:path';

@MoLow
Copy link
Member

MoLow commented Sep 6, 2023

@OshriAsulin I resolved the suggestions you have fixed, there are two more issues raised that are not taken care of:

  • path should be replaced with node:path
  • import process from 'node:process'; can be removed since process is a global

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

  • import process from 'node:process'; can be removed since process is a global

@MoLow see

node/.eslintrc.js

Lines 108 to 111 in ee391f3

{
name: 'process',
message: 'Import process instead of using the global',
},

@MoLow
Copy link
Member

MoLow commented Sep 6, 2023

@aduh95 ok then I guess we should add it to the CJS example

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

@aduh95 ok then I guess we should add it to the CJS example

No, I don't think so, no one said that the examples need to be a line-by-line equivalent, and it's consistent with most snippets in the docs: CJS ones rely on global.process, ESM ones import it from node:process.

@atlowChemi
Copy link
Member

@OshriAsulin The commit message of first commit should be changed to start with lower case character after the submodule (doc: Fix -> doc: fix)

Could you rebase and change commit message?
Let me know if you need any help 🙂

@aduh95
Copy link
Contributor

aduh95 commented Sep 6, 2023

I'd suggest using doc: fix missing imports in `test.run` code examples. Note it can be done by whoever lands the PR, it's totally optional for you to do it.

The script was missing necessary imports for the `run`
function and the `path` module, causing it to fail.
This commit adds the missing imports and resolves the issue.

- Import `run` from the appropriate module.
- Import `path` to resolve file paths.

The script should now run without errors.
This commit enhances the script by addressing missing imports for the 'run' function and the 'path' module, which previously resulted in script failure. The following improvements have been made:

Imported 'run' from the appropriate module via require to ensure correct functionality.
Imported 'path' via require to facilitate proper file path resolution.
Imported 'process' via require to include this essential module.
These changes resolve the issue of missing dependencies execution.
In this commit, I've refactored the import statements and destructuring assignments in the codebase to enhance code organization and maintainability. The primary changes are as follows:

1. Replaced the import statement:
   - From: `import { tap, run } from 'node:test/reporters';`
   - To: 
     - `import { run } from 'node:test';`
     - `import { tap } from 'node:test/reporters';`

2. Reorganized destructuring assignments:
   - From:
     ```
     const { tap, run } = require('node:test/reporters');
     const process = require('process');
     ```
   - To:
     ```
     const { run } = require('node:test');
     const { tap } = require('node:test/reporters');
     ```
In this commit, I update  import and require statements throughout the codebase to use 'node:path' instead of 'path'.
@atlowChemi atlowChemi added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6919d72 into nodejs:main Sep 6, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6919d72

@MoLow
Copy link
Member

MoLow commented Sep 6, 2023

@OshriAsulin thanks for your contribution!

ruyadorno pushed a commit that referenced this pull request Sep 28, 2023
The script was missing necessary imports for the `run`
function and the `path` module, causing it to fail.
This commit adds the missing imports and resolves the issue.

- Import `run` from the appropriate module.
- Import `path` to resolve file paths.

The script should now run without errors.

PR-URL: #49489
Fixes: #49488
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This was referenced Sep 28, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
The script was missing necessary imports for the `run`
function and the `path` module, causing it to fail.
This commit adds the missing imports and resolves the issue.

- Import `run` from the appropriate module.
- Import `path` to resolve file paths.

The script should now run without errors.

PR-URL: nodejs#49489
Fixes: nodejs#49488
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Example for running the test runner manually is broken
7 participants