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

Enhancements to custom reporter #5437

Closed
2 tasks done
Tracked by #5609
kanej opened this issue Jun 27, 2024 · 4 comments
Closed
2 tasks done
Tracked by #5609

Enhancements to custom reporter #5437

kanej opened this issue Jun 27, 2024 · 4 comments
Assignees
Labels
status:ready This issue is ready to be worked on v-next A Hardhat v3 development task

Comments

@kanej
Copy link
Member

kanej commented Jun 27, 2024

In Hardhat v3 we expect the default experience will include the ./v-next/hardhat-node-test-reporter as part of the install.

This is a placeholder for improvements to the custom reporter.

PRs

Done

  • Add a README.md explaining the project, how to install it, use it, and that it aims to have an output similar to mocha's default reporter. On top of that, explain the custom features that we add.
    The general architecture of the project is a bit rough, I think it could use some documentation/explanations. Things about separation of the main generator with the UI are there, but maybe a bit messy.

In Review

  • Testing, mostly integration tests. Most edge cases aren't tested. Just to list a few: running with --test-only, tests failing an a before/after/beforeEach/afterEach, a test file throwing top level, a describe callback throwing, no tests found, --test-only excluding every tests, multiple tests files, tests files importing other tests files, errors with and without cause.

In Progress

  • The error formatting is somewhat fragile, as it tries to do util.inspect, then split apart the stack trace from the message, and maybe add a diff. This leads to a few edge cases, and doesn't properly support error causes. I think we can take more control over it and parse the stack trace manually (ideally without any dependency except if we find a well-maintained ~0-dep one), to make it more robust.
  • We should clean up the internal node stack trace frames (related to the above).

TODO

  • We should normalize paths between windows/linux (i.e. path.sep), and relative/absolute paths. Ideally, showing absolute paths, but it is relative in case they are within the CWD.
@github-project-automation github-project-automation bot moved this to Backlog in Hardhat Jun 27, 2024
@github-actions github-actions bot added the status:ready This issue is ready to be worked on label Jun 27, 2024
@kanej kanej added the v-next A Hardhat v3 development task label Jun 27, 2024
@kanej kanej moved this from Backlog to To-do in Hardhat Jun 27, 2024
@alcuadrado
Copy link
Member

This reporter was hacked together by me, based on a project I found offline and some work by Chris.

It needs general polishing and better testing, and we also need to add some custom functionality.

These are the polishing and testing things we need to improve:

  1. Add a README.md explaining the project, how to install it, use it, and that it aims to have an output similar to mocha's default reporter. On top of that, explain the custom features that we add.
  2. The general architecture of the project is a bit rough, I think it could use some documentation/explanations. Things about separation of the main generator with the UI are there, but maybe a bit messy.
  3. Testing, mostly integration tests. Most edge cases aren't tested. Just to list a few: running with --test-only, tests failing an a before/after/beforeEach/afterEach, a test file throwing top level, a describe callback throwing, no tests found, --test-only excluding every tests, multiple tests files, tests files importing other tests files, errors with and without cause.
  4. The error formatting is somewhat fragile, as it tries to do util.inspect, then split apart the stack trace from the message, and maybe add a diff. This leads to a few edge cases, and doesn't properly support error causes. I think we can take more control over it and parse the stack trace manually (ideally without any dependency except if we find a well-maintained ~0-dep one), to make it more robust.
  5. We should clean up the internal node stack trace frames (related to the above).
  6. We should normalize paths between windows/linux (i.e. path.sep), and relative/absolute paths. Ideally, showing absolute paths, but it is relative in case they are within the CWD.

Custom functionality

Customizing how errors are displayed

We want to be able to show custom error details in some cases, in particular, when the error comes from a failed transaction that we run within EDR.

Instead of this reporter knowing about it, we should provide a generic solution to customize the how the details of an error are displayed.

I imagine something like checking if the error has a special property defined (e.g. Symbol.for("@nomicfoundation/hardhat-node-test-reporter/custom-details")) with an async function, calling it, and using its result as the error details.

@galargh
Copy link
Member

galargh commented Aug 28, 2024

Instead of this reporter knowing about it, we should provide a generic solution to customize the how the details of an error are displayed.

I imagine something like checking if the error has a special property defined (e.g. Symbol.for("@nomicfoundation/hardhat-node-test-reporter/custom-details")) with an async function, calling it, and using its result as the error details.

I really liked this idea and even explored, at length, if we could piggy-back on Custom Inspection Functions on Objects because that would give us access to a wide array of nice, standard formatting for all sorts of error objects (AssertionError, for example, implements custom inspection). Unfortunately, I discovered that when the node test runner executes tests in process isolation mode (the default), unsurprisingly, we don't have access to class methods :( There is an experimental isolation flag in the node's HEAD, which allows turning off isolation, but I don't think that's something we should pursue.

So, for custom error formatting, I'm afraid we might have to either put a pre-formatted message on the error object as a property before it is propagated out of a child test process or embed the knowledge on how to format specific errors in the custom reporter.

In e94c4bf I did a crude exploration of how we could potentially recreate concrete error classes in the reporter (to be able to access custom formatters defined in them) while keeping the dependencies on the error providers optional. Still, I'm not sure how I feel about it yet.

@alcuadrado
Copy link
Member

Unfortunately, I discovered that when the node test runner executes tests in process isolation mode (the default), unsurprisingly, we don't have access to class methods :(

Does node serialize all the data that's present in the error? By that I mean, if we add custom fields with json-serializable values, do we get those? If we do, we can create some convention around it.

@galargh
Copy link
Member

galargh commented Sep 2, 2024

Unfortunately, I discovered that when the node test runner executes tests in process isolation mode (the default), unsurprisingly, we don't have access to class methods :(

Does node serialize all the data that's present in the error? By that I mean, if we add custom fields with json-serializable values, do we get those? If we do, we can create some convention around it.

Yes, that we can definitely do. We do have access to serializable values. That's exactly how AssertionError instances are handled, for example, by the inspect function.

@kanej kanej closed this as completed Sep 18, 2024
@github-project-automation github-project-automation bot moved this from In Review to Done in Hardhat Sep 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:ready This issue is ready to be worked on v-next A Hardhat v3 development task
Projects
Archived in project
Development

No branches or pull requests

3 participants