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

Skip schema validation/v1 #2074

Closed
wants to merge 2 commits into from

Conversation

inashivb
Copy link
Member

@inashivb inashivb commented Oct 3, 2024

Just a suggestion following up on #2068 (comment)
I've taken up #2073 and added the feature there to show how it works. (Note that this feature should actually just be used for older version tests that cannot get that schema update)

This is for cases where because of broken schema, a test would just always fail. But, we can be lenient about the schema and ensure that the output actually matched in these cases.

Thoughts?

@catenacyber
Copy link
Collaborator

Looks weird to me : I think the test fails.
You can add some output to describe the reason of the failure (or failures plural) if you want to post process these and allow tests that fail only because of schema.

You can also skip the schema validation by not having the eve-validator somehow, right ?

@jufajardini
Copy link
Contributor

Looks weird to me : I think the test fails. You can add some output to describe the reason of the failure (or failures plural) if you want to post process these and allow tests that fail only because of schema.

This is an interesting idea, too.
Set default to JSON schema failure not to be fatal, just issue a warning, and test runs normally.
But still have at least one CI check with schema validation as fatal, so these failures would not slip by.

@inashivb
Copy link
Member Author

inashivb commented Oct 4, 2024

Looks weird to me : I think the test fails. You can add some output to describe the reason of the failure (or failures plural) if you want to post process these and allow tests that fail only because of schema.

Noice and better. Thanks a lot! 🙇🏽‍♀️

You can also skip the schema validation by not having the eve-validator somehow, right ?

hmm only found yesterday that there's a command line option to do this. So, apparently, given that we anyway won't check old versions in the CI, one could easily run s-v w --no-validation on old versions.

So, all in all, moving forward, if we want, we should have multiple failure reasons logged. and, that's about it.

@inashivb inashivb closed this Oct 4, 2024
@inashivb inashivb deleted the skip-schema-validation/v1 branch October 4, 2024 05:43
@jufajardini
Copy link
Contributor

You can also skip the schema validation by not having the eve-validator somehow, right ?

hmm only found yesterday that there's a command line option to do this. So, apparently, given that we anyway won't check old versions in the CI, one could easily run s-v w --no-validation on old versions.

TIL (or re-learned) about the --no-validation option. I'll use that for one of my PRs, I guess.

@inashivb
Copy link
Member Author

TIL (or re-learned) about the --no-validation option. I'll use that for one of my PRs, I guess.

yeah it seems helpful. Should have guessed its availability since Jason worked on it too. He is generally very mindful of common required options for usability. :)

@jufajardini
Copy link
Contributor

TIL (or re-learned) about the --no-validation option. I'll use that for one of my PRs, I guess.

yeah it seems helpful. Should have guessed its availability since Jason worked on it too. He is generally very mindful of common required options for usability. :)

:P
Although that option didn't work as I expected it to xD

@inashivb
Copy link
Member Author

TIL (or re-learned) about the --no-validation option. I'll use that for one of my PRs, I guess.

yeah it seems helpful. Should have guessed its availability since Jason worked on it too. He is generally very mindful of common required options for usability. :)

:P Although that option didn't work as I expected it to xD

oh. Then, maybe we should indeed have ticket for that

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

Successfully merging this pull request may close these issues.

3 participants