Unit test is wrong

[airween]

The linter has a few unit tests, but the [test_cli`(https://github.com/coreruleset/crs-linter/blob/main/tests/test_cli.py) is wrong.

It expects the main() returns with 0, but

• most tests under examples/ contains some error intentionally to demonstrate how the linter works
• therefore the main() mustn't return with 0
• the reason is simple: in the unit test we passed wrong path of tests here and here: the ../ isn't necessary
• the other problem is that if the linter gets a non-existent path for rules, it eats silently, there is no report

I can send a PR, but how do we want to solve this?

My suggestion is:

• add check against -r argument; if there is a nonexistent file/path, so the files is an empty list, then linter exists immediately
• remove ../ from arguments' -r of cli's unit test
• change assert == 0 to assert == 1

Question: now the APPROVED_TAGS and TEST_EXCLUSIONS are empty (temporary) files; because all listed tests contain some error (intentionally), we don't need to fill them, those need only to start the linter, right?

[theseion]

• add check against -r argument; if there is a nonexistent file/path, so the files is an empty list, then linter exists immediately

Yes

• remove ../ from arguments' -r of cli's unit test

Yes

• change assert == 0 to assert == 1

I don't agree. Whether main exits with (or raises an) error depends on the scan. There should be multiple tests for different configurations of the CLI and the exit code will depend on that configuration (and the checked files). I also think that checking the exit code makes sense but that it would make much more sense to check for errors (of course, that would require some bigger changes). All in all I don't see a lot of value in this test to begin with.

I think it makes more sense to test that main calls other functions as expected for a given configuration. Simply mock the functions that should be called and then check that they have been called with the expected arguments.

[airween]

• first item...

Yes

• second item...

Yes

Thanks,

• change assert == 0 to assert == 1

I don't agree. Whether main exits with (or raises an) error depends on the scan. There should be multiple tests for different configurations of the CLI and the exit code will depend on that configuration (and the checked files). I also think that checking the exit code makes sense but that it would make much more sense to check for errors (of course, that would require some bigger changes). All in all I don't see a lot of value in this test to begin with.

I think it makes more sense to test that main calls other functions as expected for a given configuration. Simply mock the functions that should be called and then check that they have been called with the expected arguments.

So the best thing that we should do is:

• introduce unique error codes for each check method
• if we keep this way of checking (I mean we don't introduce more tests and don't separate them/the existing ones), then we expect definitely a specified return value, right?

[theseion]

Yes, I agree.