search

sketch-hq / nicene

Additional Credo checks
MIT License
39 stars 7 forks source link

`TestsInTestFolder` is over-agressive #21

Open halostatue opened 4 years ago

halostatue commented 4 years ago

This check asserts the whether "lib" in Path.split(source_file.filename) is true. However, this is finding cases like: test/lib/context/employees_test.exs, which is technically correct (and indicates that the test probably belongs in a different folder), but is not correct because the test is under the test/ folder.

It seems to me that the condition should be that a test file is, first and foremost, found outside of the test/ directory, or if that’s not preferred, that there’s a second check to make sure that the file’s path doesn’t begin with test/.

uesteibar commented 3 years ago

Hi @halostatue! Not sure I'm following, test/lib/context/employees_test.exs would (according to the rule) indeed be wrong and should be test/context/employees_test.exs right? I'm sure I'm missing something here tho, so if you could clarify that it'd be very helpful.

halostatue commented 3 years ago

It took me a bit to recall what I meant from months ago. However, it should not be incorrect to have test/lib/context/employees_test.exs, even if it’s a little unusual. More problematic might be something like test/module/lib/function_test.exs that would be testing lib/module/lib/function.ex (Module.Lib.Function). The test here should probably be verifying that the first path part for a test file is test/ and not lib/.

uesteibar commented 3 years ago

Ah I see what you mean! Yes that is correct, we should indeed be checking for the first part of the path only 👍

halostatue commented 3 years ago

The main question would be whether source_file.filename is a partial path test/foo/bar.exs or a full path /home/user/projects/foo_bar/test/foo/bar.exs. If it’s the former, the test is really easy: match?("test/" <> _, source_file.filename). If it’s the latter…

uesteibar commented 3 years ago

yep, true that, I'll poke around 😸