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(process): don't ignore invalid venom testsuites #234 #235

Closed
wants to merge 2 commits into from

Conversation

youen
Copy link

@youen youen commented Mar 17, 2020

close #234

@yesnault
Copy link
Member

HI @youen

Thank you for this pull request.

We want to keep the existing behaviour as the default behaviour, as some people use some fixtures in yml format, not valid with venom, in the same directory as the test file.

But, it should be great to add a flag as --disabled-yml-check (so with the default setted to false).

Copy link

@alexispiaud alexispiaud left a comment

Choose a reason for hiding this comment

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

I have the same issue. We realise to late if we have a mistake in our yml file

@alexisvisco
Copy link

This PR should be reviewed because it's a critical bug which leads to not tested files.

@yesnault
Copy link
Member

Hi all. Yes, we are aware of this pull-request. It's under discussion and we will rework in few weeks. Our goal is to not impact existing users and if we can avoid adding a flag to enable or not a feature, it should be better.
With this PR implementation of the check, the check is done when the file is read. But, with some templating variables, the check will probably failed even if the file is ok with the good values of variables. It's not so easy to do a good check with different use case that we already know, that's why removing the feature was better than trying to get something to work, but that ruled out some uses of the template engine.

@yesnault
Copy link
Member

yesnault commented Oct 5, 2020

Ok. We discussed this issue again, we decided to rollback the current behavior which ignore errors in yml files. We don't want a flag to ignore or not: if some users want to run only a subset of yml files, they will have to filter the files list given to venom.
We will release the venom 0.28.0 with the current master, then removing the 'no-check' for the release 0.29.

@yesnault yesnault self-assigned this Oct 5, 2020
yesnault added a commit that referenced this pull request Oct 7, 2020
Users have to filter the yml files given to venom if needed

close #234
close #235

Signed-off-by: Yvonnick Esnault <yvonnick.esnault@corp.ovh.com>
@yesnault yesnault closed this in #274 Oct 7, 2020
yesnault added a commit that referenced this pull request Oct 7, 2020
Users have to filter the yml files given to venom if needed

close #234
close #235

Signed-off-by: Yvonnick Esnault <yvonnick.esnault@corp.ovh.com>
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.

bug(processor): YAML syntax error ignored
5 participants