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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Filter out 'shadowing' errors, for the humans #76

Open
nicojs opened this issue Apr 10, 2020 · 4 comments
Open

Feature request: Filter out 'shadowing' errors, for the humans #76

nicojs opened this issue Apr 10, 2020 · 4 comments

Comments

@nicojs
Copy link

nicojs commented Apr 10, 2020

Hi there 馃憢. Awesome repo you've got here!

We at the stryker mutation testing team have been looking into validation with ajv. We've come to the same conclusion, error messages directly from AJV are not really designed for humans. We're thinking of using better-ajv-errors.

However, we're also thinking of filtering out what I call 'shadowing' errors.

A shadowing error is an error that results logically from another error. Some examples:

[
  { // This is a useless error for a human
   keyword: 'type',
   dataPath: '.mutator',
   params: { type: 'string' },
   // [...]
 },
 { // => This is the most specific error. This is for humans!
   keyword: 'required',
   dataPath: '.mutator',
   params: { missingProperty: 'name' },
   // [...]
 },
 { // This is a useless error for a human
   keyword: 'oneOf',
   dataPath: '.mutator',
   params: { passingSchemas: null },
   [...]
 }
]

Or:

[
  { // This is a useless error for a human
   keyword: 'type',
   dataPath: '.logLevel',
   params: { type: 'string' },
   // [...]
 },
 { // => This is the most specific error. This is for humans!
   keyword: 'enum',
   dataPath: '.logLevel',
   params: { allowedValues: ['info', 'warn'] },
   // [...]
 },
]

A first draft of the filtering is created here:

https://github.com/stryker-mutator/stryker/blob/627d2f7e403042845bcece73c838c9447bbf522c/packages/core/src/config/validationErrors.ts#L55-L74

Do you think this filtering is useful for other projects as well? Should it be added to better-ajv-errors? Maybe as an option? Or do you want to keep it separate?

I would be willing to prepare it in a PR if you agree that this is a feature useful for all humans, nut just mutant-killing humans 馃槈.

@torifat
Copy link
Collaborator

torifat commented Aug 6, 2020

Sounds interesting, happy to give it a try as an option.

Extremely sorry about the late reply. I wasn't active for a while.

@nicojs
Copy link
Author

nicojs commented Aug 6, 2020

Thanks for the response. It is late, but I don't have any illusion of entitlement, no problem whatsoever 馃槉

We've implemented the error reporting without better-ajv-errors at the moment, but we're happy to move to better-ajv-errors when this feature is implemented.

The current filtering can be found in our code here: https://github.com/stryker-mutator/stryker/blob/2eec539ddaffa9fd65eaf1413ab29885a45ea510/packages/core/src/config/validationErrors.ts#L55-L76

Do you agree with this filtering? If so, I'd be happy to move it to here and add the necessary tests for it as well.

@torifat
Copy link
Collaborator

torifat commented Aug 6, 2020

Yes, I totally understand why they are irrelevant. I just don't want to make that a default option right away 馃檪

@nicojs
Copy link
Author

nicojs commented Aug 6, 2020

How about adding it as an option? Maybe { filterShadowingErrors: true } (default false)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants