rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.93k stars 417 forks source link

#FIXME fails silently #1088

Closed GuiHeurich closed 10 months ago

GuiHeurich commented 11 months ago

Precheck

Environment

What were you trying to do?

Run mix credo.

Expected outcome

Either a fail or a pass.

Actual outcome

A slient fail because of a #FIXME comment.

When I run mix credo, I get this result:

Screenshot 2023-11-10 at 09 58 57

To me, the above result looks like a PASS, but it was failing on CI because, I think, credo was actually returning an EXIT STATUS 2.

After running mix credo list, I realized what the issue is: credo treats #FIXME as an error, but it doesn's show it. In other words, it fails silently.

Notice how the #FIXME error shows up in yellow on the summary board:

Screenshot 2023-11-10 at 10 06 33

However, when I run mix credo list, it expands each warning and the #FIXME shows up as RED and not YELLOW:

Screenshot 2023-11-10 at 09 58 38
rrrene commented 10 months ago

I think I am missing something: You write

the #FIXME error shows up in yellow on the summary board

and

the above result looks like a PASS

but also say that Credo "fails silently".

How is it failing silently if the partial screenshot shows "found 47 software design suggestions" and exits with exit status 2?

GuiHeurich commented 10 months ago

Hiya, thanks for replying.

Let me try to explain. "The above result looks like a PASS" means that it is not, in fact, a pass, because FIXME are treated as errors. The issue is the discrepancy between the failure and the display: the display only says "47 software design suggestions" but it actually fails because FIXME is treated as an error.

Steps to reproduce:

  1. Write a FIXME comment somewhere.
  2. Run mix credo and it will show no failure but will fail on CI.
  3. Run mix credo list and you can see the FIXME failure.

In short: FIXME are considered failures, but only the list panel shows them. The summarised version doesn't show the error.

rrrene commented 10 months ago

Ah, there is probably a fundamental misunderstanding here: There is no such thing as an "error" in Credo's results.

From the docs:

Credo fails with an exit status between 1 and 127 if it shows any issues.

An exit status of 2 is the correct result for the described scenario.

Nevertheless: The fact that mix credo list uses different colors is something we should definitely revisit. That is confusing 😞 👍

GuiHeurich commented 10 months ago

OK. A "fundamental misunderstanding" is a rude way of putting this. There is no need to be rude. You are perfectly aware that I meant a failure when I said an error.

Also: the problem is not on mix credo list but on mix credo summary report.

rrrene commented 10 months ago

@GuiHeurich I am not a native speaker, so apologies if that is per se a rude way of phrasing it. How would you have preferred for this do be expressed?

There are no two types of output with Credo. Any raised "issue" that is in the output is an "issue", which leads to an exit status.

I get the impression that you think I am accusing you of something and that is not my intention. 🙏

GuiHeurich commented 10 months ago

It's fine, don't worry about it - I'm not a native speaker either. Good luck with all your work.