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

fix(vitest): json reporter location should include file #5106

Closed
wants to merge 1 commit into from

Conversation

vird
Copy link

@vird vird commented Feb 2, 2024

Description

JSON reporter returns location of error, but there are multiple files. What is origin of line, column? What file?
So I decided to fix that

NOTE. I don't know how to fix test snapshots, so they will run on each machine. File returns absolute path, so matching with snapshot is not possible, I guess

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Feb 2, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 4915030
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65bd62d0e7d0960008eb8b70

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 5, 2024

Thanks for the PR. FYI, there were some related discussions in

and it looks like including stacktrace in failureMessages is currently considered.

Including location.file also makes sense to me, but could you also check if you might also want stacktrace?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 5, 2024

NOTE. I don't know how to fix test snapshots, so they will run on each machine. File returns absolute path, so matching with snapshot is not possible, I guess

Btw, one way to deal with this type of non-determinism in snapshots is to replace "dirname" with some placeholder value like this:

expect(stderr.replace(resolve(__dirname, '..'), '<root>')).toMatchSnapshot()

@@ -193,6 +193,9 @@ export class JsonReporter implements Reporter {
if (!frame)
return

return { line: frame.line, column: frame.column }
let file = ''
Copy link
Member

Choose a reason for hiding this comment

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

I don't think having an empty string here is a good idea

Copy link
Author

Choose a reason for hiding this comment

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

return file == '' case should be unreachable. Because there should be no error without stack
So it's mostly for convincing typescript that all is ok anyway
If there are line and column, but there is no file, then that's strange input to this function. Strange input - strange output

Copy link
Member

Choose a reason for hiding this comment

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

If you just want to satisfy TS, you can use !

@sheremet-va
Copy link
Member

Jest doesn't have a file property. It is inferred from the test name. Vitest reports incorrect location property. This should be fixed by #5434

@sheremet-va sheremet-va closed this Apr 5, 2024
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.

None yet

3 participants