search

codechecks / monorepo

Open source platform for code review automation ✅
https://codechecks.io/
98 stars 19 forks source link

Fail if any check has been failures #22

Closed timocov closed 5 years ago

timocov commented 5 years ago

Currently if you run codechecks it always exited with code 0, even if any check is failed. I'm not sure, but I believe CLI should fail in that case as well.

timocov commented 5 years ago

Or maybe don't fail if run is for PR and fail for local runs only...

krzkaczor commented 5 years ago

Yeah, propagating fail locally makes total sense to me, thanks for this idea. On CI I don't think that this is a necessary behaviour since PR will be marked as failed by Checks API.

krzkaczor commented 5 years ago

@MichalZalecki this is something that you can work on

MichalZalecki commented 5 years ago

@krzkaczor Will look into that 👌

MichalZalecki commented 5 years ago

btw. you can assign me to it's clear this feature is worked on

krzkaczor commented 5 years ago

@MichalZalecki the problem is unless you make a comment I cant 😆

MichalZalecki commented 5 years ago

@krzkaczor I see how it might be better to implement it as an opt-in behavior with default consistent with what CI providers are doing. The flag could be called --fail-fast (RSpec) or --bail (Jest).

krzkaczor commented 5 years ago

@MichalZalecki yeah I agree, this might be a little bit more flexible. We just need to make sure to document it properly in docs (people can search for something around git hooks subject).

As for the name, I think --fail-fast or --bail have a little bit different meaning because this option would be more about error propagation... maybe --exitOnError? But I don't have a strong opinion about it...

timocov commented 5 years ago

I'm not sure that --fail-fast is what I'm looking for (but it's helpful though). I don't want to stop checks execution if one on them is failed - I want to fail the whole execution if one of them is failed, but all others should report their status as well.

MichalZalecki commented 5 years ago

Ok, I understand. I might be me missing something, but we would have to start keeping reports from check plugins in the state which we don't ATM.

krzkaczor commented 5 years ago

@MichalZalecki @timocov yes I think we got two features mixed in this issue.

First one is what @timocov originally described here. One can rephrase it as "if any check(plugin) created report with status === "failure" then exit code of CLI should be !== 0. This is something that could be hidden behind --exitOnError flag. If anyone has a better name let me know ;) As @MichalZalecki mentioned, it would require us to keep a local state of submitted reports somewhere.

The second feature, is what @MichalZalecki implemented which is equivalent of mocha's --bail. "if any check(plugin) created a report with status === "failure" then CLI should stop executing next check".

Both are useful and both should be implemented. I am gonna review @MichalZalecki PR and see if it makes sense to add (1) feature there or we should merge it first and then work on (1) in a separate stream of work.

timocov commented 5 years ago

I believe this issue should be re-opened :)

krzkaczor commented 5 years ago

@timocov on one hand the feature mentioned originally is not solved but on the other @MichalZalecki 's solution returns non-zero error code in case of failure so it already should solve some problems related to this. Especially it can now be used as git hook. But anyway, I am keeping this open.

timocov commented 5 years ago

Especially it can now be used as git hook

Yep, but it's good to show all errors related to the all code (like linters do for example), not only the first error in the git hook and "ask" developers to fix it and then run all hooks again to see what's next is failing - just to save our time and show all errors related to the changes :)

IMO fail-fast is good when you just need to detect whether everything is good or not, without thinking about what exactly is going wrong.

krzkaczor commented 5 years ago

Finally released as @codechecks/client@0.1.10 Kudos to @MichalZalecki.

There is a new flag --with-exit-status or simply -x.

timocov commented 5 years ago

@MichalZalecki @krzkaczor Thank you guys for your work! Codechecks is really awesome tool.

I just read your discussion in the PR - is there a reason why this flag is not enabled by default for non-CI runs (only for them)?

MichalZalecki commented 5 years ago

I'm also leaning towards making it a default behavior as discussed in #46

krzkaczor commented 5 years ago

I got your feedback loud and clear (also @recmo noted that this would be expected behavior for him). I guess I was too focused on my initial, outdated vision.

I am fine with making this default behavior and introducing the opposite flag in 0.2.x.