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

test: add resolve-plugin-list tests #5255

Merged
merged 2 commits into from
May 21, 2024
Merged

test: add resolve-plugin-list tests #5255

merged 2 commits into from
May 21, 2024

Conversation

kanej
Copy link
Member

@kanej kanej commented May 20, 2024

Add tests on the dependency resolution of plugins.

Resolve #5254.

@kanej
test: add resolve-plugin-list tests
90f801e 
Add tests on the dependency resolution of plugins.

Resolve #5254.
@vercel Vercel
Copy link

vercel bot commented May 20, 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 20, 2024 7:59pm

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 20, 2024 •
edited

⚠️ No Changeset found

Latest commit: c78afd9

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

@kanej kanej requested a review from alcuadrado May 20, 2024 16:11
@github-actions github-actions bot added the status:ready This issue is ready to be worked on label May 20, 2024
@kanej kanej linked an issue May 20, 2024 that may be closed by this pull request
Polish and test the plugin resolution/ordering logic #5254
Closed
@kanej kanej added the no changeset needed This PR doesn't require a changeset label May 20, 2024
alcuadrado
alcuadrado reviewed May 20, 2024
View reviewed changes
v-next/core/test/plugins/resolve-plugin-list.ts
const copy = { id: "dup" };

assert.throws(
() => resolvePluginList([a, copy]),
Copy link
Member

Choose a reason for hiding this comment

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

It should throw a HardhatError and test that instead of the string.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the Hardhat error for duplicate plugins under GENERAL in the error descriptors. Let me know if that is not the category you were expecting.

@alcuadrado
Copy link
Member

This looks good. I left a minor comment, but otherwise looks good.

@kanej
improvement(plugins): use a HH error for duplicate id
c78afd9 
Adds a plugin loading duplicate id error under the `general` category
of Hardhat errors and uses that inside `resolve-plugin-list`.
@kanej kanej requested a review from alcuadrado May 20, 2024 20:04
@kanej kanej merged commit 399e449 into v-next May 21, 2024
39 checks passed
@kanej kanej deleted the test/plugin-resolution branch May 21, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alcuadrado alcuadrado alcuadrado approved these changes

Assignees
No one assigned
Labels
no changeset needed This PR doesn't require a changeset status:ready This issue is ready to be worked on
Projects
Hardhat
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Polish and test the plugin resolution/ordering logic
2 participants
@kanej @alcuadrado