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.91k stars 414 forks source link

Fix the path for included files in umbrella apps #1133

Closed tomasz-tomczyk closed 3 months ago

tomasz-tomczyk commented 3 months ago

Hello! I ran into an issue where Credo.Check.Refactor.PassAsyncInTestCases would not flag issues when running mix credo from the root of an umbrella application, but cd-ing into apps/app_web and running it from there would correctly flag the missing async option.

We only have a single .credo.exs config at the root and I replicated the issue with the barebones config:

%{
  configs: [
    %{
      name: "default",
      checks: %{
        enabled: [{Credo.Check.Refactor.PassAsyncInTestCases, []}]
      }
    }
  ]
}

Before this change, inside the Credo.Sources.find_in_dir the value for included_patterns would compile down to:

["/Users/tom/my_umbrella_app/test/**/*_test.exs"]

But that would skip everything that was under my_umbrella_app/apps/**/test

I see this was intentionally changed in https://github.com/rrrene/credo/commit/ac76775a37bceea6afe66c00d1647fe60c46d755 so maybe I'm wrong to submit this PR and would appreciate feedback! Changing this locally did make the check work from both the root and the individual umbrella app directories though.

rrrene commented 3 months ago

Thx for pointing this out. I will take a closer look soon. :+1:

tomasz-tomczyk commented 3 months ago

Thanks for taking a look!

I've reproduced in a minimal example in this repo: https://github.com/tomasz-tomczyk/credo-async-example

rrrene commented 3 months ago

@tomasz-tomczyk I had to change the way the files were addressed, because the check for wrong file extensions would otherwise yield issues for files like lib/foo/foo_case_test.ex which we avoided in the past.

Please check out the RC to see if this works for you: https://github.com/rrrene/credo/releases/tag/v1.7.7-rc.0 :+1:

tomasz-tomczyk commented 3 months ago

Ah okay, got it. Thanks! It works for us yes 👍