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

infra: new test #35

Merged
merged 4 commits into from
Oct 23, 2024
Merged

infra: new test #35

merged 4 commits into from
Oct 23, 2024

Conversation

AugustinMauroy
Copy link
Member

#34

@AugustinMauroy
Copy link
Member Author

cc @rvagg

@rvagg
Copy link
Member

rvagg commented Oct 21, 2024

I'm not really up on the new node test runner, is this directory structure essential to make that work? Is src and src/__test__ a mandatory thing for that?

@AugustinMauroy
Copy link
Member Author

AugustinMauroy commented Oct 21, 2024

I'm not really up on the new node test runner, is this directory structure essential to make that work? Is src and src/test a mandatory thing for that?

absolutely not, but having a directory that differentiates between source code and tests seems important to me.
So the choice of file structure is completely arbitrary.
https://nodejs.org/docs/latest/api/test.html#running-tests-from-the-command-line

@rvagg
Copy link
Member

rvagg commented Oct 21, 2024

ok, but __test__ isn't really idiomatic, and putting it inside src even less so

how about just test and src?

it is nice to see the tests move out of the source files though

@AugustinMauroy
Copy link
Member Author

Updated

@rvagg
Copy link
Member

rvagg commented Oct 22, 2024

sorry to be picky @AugustinMauroy, would you mind switching to test instead of tests? it's a bit more typical and even the new tests API docs say it uses it when looking for tests (I know your filenames do this).

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌 (holding my nose at the package-lock, it's not unreasonable for this package I suppose)

@rvagg rvagg merged commit dac84eb into nodejs:main Oct 23, 2024
7 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 23, 2024
## [1.7.4](v1.7.3...v1.7.4) (2024-10-23)

### Trivial Changes

* **ci:** give release job permission to write to repo ([36c7702](36c7702))
* **deps:** bump actions/setup-node from 4.0.3 to 4.0.4 ([#31](#31)) ([91d60f1](91d60f1))
* **test:** use inbuilt node test package ([#35](#35)) ([dac84eb](dac84eb))
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.

3 participants