-
Notifications
You must be signed in to change notification settings - Fork 211
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
Don't fail empty results if failOnError is False #220
base: main
Are you sure you want to change the base?
Conversation
Is it even a failure if no test results have been found? It should rather be a warning in my opinion. This behavior already caused issues for me in the past, because I'm using a reusable workflow for my CI purposes. One of the projects did not have any tests (because it was just an autogenerated wrapper for something else) and the test report step failed because of this. On the other hand, I definitely want "failOnError" enabled in general for my projects. My proposal is to change this error to a warning regardless of the "failOnError" value (cc @dorny). Not even sure if it's worth increasing complexity with a new "failIfNoTestsFound" input, but that might be something to consider if one really need to fail in this case. I don't see a valid usecase for this at the moment, but maybe I'm missing a valid point. |
When you say "this behavior", which do you mean? We have two kinds of failures we can report on here.
And we have two places we can report them
I believe its best to keep these clearly separate. 1 with 1 and 2 with 2. Then, For example, if no test results are found, because perhaps the file name was misspelled or so, i think it is good if the action marks itself as failed. Then, if you're unhappy with that behavior, its best to use the For case 2-2, that is where you may need an additional flag to control when you want that to fail. I actually just tried |
This is the behavior I slightly disagree with 😓 If the file is misspelled, this might indicate a bug in the CI workflow, but might as well be perfectly fine (like in the scenario I described in my previous comment). Therefore I think we should change this to a "warning". I can live with using the |
Hi guys, Is any of you changed his mind ? |
Guys, I merge this PR #243. |
This also changes how the check is created, failing the check on failing tests. This resolves #217 and also resolves #161.
If people would actually still like the check itself to pass on failing tests, I would recommend adding that as an additional parameter to the action (
failCheckOnFailure
?), but I don't trust my own TS skills enough to do that. However, my guess is that #214 was a misunderstanding of what #161 wanted to do, and not many people actually want what #214 adds.