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.

Sign up for GitHub

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

Jump to bottom

Better node:test reporter and scripts #5249

Merged
merged 7 commits into from
May 31, 2024
Merged

Better node:test reporter and scripts #5249

merged 7 commits into from
May 31, 2024
+163 −74

Conversation

alcuadrado
Copy link
Member

@alcuadrado alcuadrado commented May 17, 2024 •
edited

This PR introduces a few changes to make working with node:test in v-next easier:

  1. It changed the reporter from the builtin spec to our own test reporter.
  2. It adds a test:only npm script, to run the tests with .only. Just run pnpm test:only.
  3. It adds a CI job to ensure that all the npm scripts in v-next/ are consistent with each other.
  4. Removes glob from our peer dependencies, as node:test has native support for globs in node 22

@alcuadrado alcuadrado requested a review from schaable May 17, 2024 20:42
@vercel Vercel
Copy link

vercel bot commented May 17, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 31, 2024 0:48am

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 17, 2024 •
edited

⚠️ No Changeset found

Latest commit: 88988b3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the status:ready This issue is ready to be worked on label May 17, 2024
@schaable
Copy link
Contributor

we could add a linter rule to enforce strict assertion mode by importing import assert from 'node:assert/strict'; instead of import assert from 'node:assert';

@alcuadrado
Copy link
Member Author

alcuadrado commented May 18, 2024 •
edited

Let's wait before merging this. See: voxpelli/node-test-pretty-reporter#16

Using our own test reporter instead.

@alcuadrado alcuadrado changed the base branch from v-next to build-system-solcjs May 31, 2024 00:11
@alcuadrado
Copy link
Member Author

we could add a linter rule to enforce strict assertion mode by importing import assert from 'node:assert/strict'; instead of import assert from 'node:assert';

This was implemented in another PR

@alcuadrado
Copy link
Member Author

I made this PR depend on #5299 because that was preventing me from passing the CI tests

@alcuadrado alcuadrado requested a review from schaable May 31, 2024 00:25
@alcuadrado alcuadrado closed this May 31, 2024
@alcuadrado alcuadrado reopened this May 31, 2024
schaable
schaable reviewed May 31, 2024
View reviewed changes
v-next/hardhat-build-system/package.json
"test:coverage": "c8 --reporter html --reporter text --all --src src glob --cmd=\"node --import tsx/esm --test\" \"test/**/*.ts\"",
"test": "node --import tsx/esm --test --test-reporter=@nomicfoundation/hardhat-node-test-reporter \"test/**/*.ts\"",
"test:only": "node --import tsx/esm --test --test-only --test-reporter=@nomicfoundation/hardhat-node-test-reporter \"test/**/*.ts\"",
"test:github": "node --import tsx/esm --test --test-reporter=@reporters/github --test-reporter-destination=stdout --test-reporter=@nomicfoundation/hardhat-node-test-reporter --test-reporter-destination=stdout \"test/**/*.ts\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this line correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what exactly you mean by "this". Can you expand a bit?

Copy link
Contributor

@schaable schaable May 31, 2024 •
edited

Choose a reason for hiding this comment

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

yes, sorry. The test:github script has --test-reporter and --test-reporter-destination set twice. It looks like an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, no, it's intentional. You can have multiple reporters. In this case, we have the github one to add comments into our PRs, and ours to have an output during our test runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤯

schaable
schaable reviewed May 31, 2024
View reviewed changes
v-next/hardhat-node-test-reporter/integration-tests/index.ts
@@ -1,24 +1,22 @@
import { readFileSync, readdirSync, statSync } from "node:fs";
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why we're using the sync versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't run anything in parallel, so there's no value in using async versions, and it was just complicating a filter() call. If this were a test run by the actual test runner, I'd use async versions, but it's a top-level single-purpose script.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the call, but I'm uneasy about starting to use node:fs directly. If we add exceptions, this practice might spread, and we could miss out on good error handling and other checks. Do you think it's worth adding the sync versions to the utils?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see. Your question is not about sync/async, but I guess you want to know why not the utils package.

That's not possible because it would introduce a circular dependency between projects, and the typescript will complain.

The reporter used to use the utils package, but as soon as I started using the reporter in the utils package, that error appeared.

I think it's better to have a nicer reporter in -utils than to avoid importing fs directly here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Makes sense under that scenario 👍

schaable
schaable reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@schaable schaable left a comment

Choose a reason for hiding this comment

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

Left a couple questions

Base automatically changed from build-system-solcjs to v-next May 31, 2024 16:14
@alcuadrado alcuadrado merged commit 0decf9a into v-next May 31, 2024
57 checks passed
@alcuadrado alcuadrado deleted the better-node-test-reporter branch May 31, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schaable schaable schaable approved these changes

Assignees
No one assigned
Labels
status:ready This issue is ready to be worked on
Projects
Hardhat
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@alcuadrado @schaable