Make a great experience testing ignored files

elm-review v2.11 introduced a way to know whether a file is ignored. The aim for this capability is largely for performance concerns: avoiding collecting information from ignored files and/or doing expensive computations to report errors that will be ignored anyway.

I tried using it on a few rules, and while it seems fairly easy to use, it's also very scary because the tests are not helping us out, as none of my existing tests are testing with ignored files. It's really hard to figure the impact of a code like is isFileIgnored then Dict.empty else Dict.singleton moduleName moduleContext.something in a function like fromModuleToProject.

Because it's scary to use, I'm probably not going to use it (too bad for performance improvements) or do it and hope for people to report issues (that are going to be hard to debug).

Proposed solution

My idea is to have elm-review automatically try the rule in the original conditions, then run again with some files being ignored and check that the results are the same. This would only be necessary if the rule looks at whether ignored files (which we know through RequestedData). To prevent a false sense of security and to do as much as possible, all combinations of ignored/not-ignored files should be attempted.

Fuzzing would also be possible but the API for elm-review creates an Expectation, not a Test, so this would require API changes. Also, we would likely run the rule way more times than necessary with fuzzing than when running all possible combinations. If N is the number of files, there would be N^2 number of combinations to try. Fuzz tests usually run 100 different tests, and we'd run 64 tests if there are 6 different files, 128 tests if we have 7, which seems like a very high number in general.

Most tests cases don't have many files, so this should in practice not be a huge testing performance issue, though I guess we'll only see this if we try this out.

Impact on data extracts

Ignored files can impact data extracts. I think this is nice because we might we might want to remove a lot of cruft around generated files for instance (though that could also be passed as the rule's configuration?).

My proposal is for data extracts to only be

Should we only test the case where nothing is (additionally) ignored? That sounds reasonable, especially since the opposite would require people to add expected variants for a large number of cases for each test. It's probably better to let rule authors intentionally test whether/how ignored files impact the data extract, though I don't know if they will in practice.

Way to disable this

I am having a little trouble thinking of scenarios (except for data extracts) where one might want to alter the results based on the ignored files. I could see the details of an error changing though, for instance a rule that reports a function and shows where it is used ("this function is used in src/A.elm, src/B.elm, ..."). Maybe we would like to not include ignored files in this list.

Maybe a test function like |> Review.Test.dontTestWithIgnoredFiles (name to be reworked) could be added between the Review.Test.run* and Review.Test.expect* usages to prevent this behavior from kicking in.

Implementation

For the tests to know whether a rule knows about which files are ignored, we'd need to add and expose a Review.Rule.ruleKnowsAboutIgnoredFiles function like this:

{-| Indicates whether the rule knows about which files are ignored.

You should not have to use this when writing a rule.

-}
ruleKnowsAboutIgnoredFiles : Rule -> Bool
ruleKnowsAboutIgnoredFiles (Rule rule) =
    -- TODO Breaking change: This should be an internal detail, not shown to the user
    let
        (RequestedData requestedData) =
            rule.requestedData
    in
    requestedData.ignoredFiles

(very similar to Review.Rule.ruleProvidesFixes)

In Review.Test, I think that this requires a rework of the Review.Test.ReviewResult type, to hold the project data and the rule, so that we can run variations.

The rest of the details I have not yet figured out. I know that we'd likely need to change the failure message of the tests to make it explicit in which permutation a discrepancy occurs. Ideally, compare it with a test where things were okay. Rough example:

RULE REPORTED LESS ERRORS THAN EXPECTED

I expected to see 5 errors for module `MyModule` but only found 4.
Here are the 1 I could not find:

  - `Remove the use of `Debug` before shipping to production`

---

This problem only occurs when trying to ignore some files. It looks like this problem starts occurring when `src/A.elm` is ignored.

Through `Review.Rule.withIsFileIgnored`, you are checking which files are ignored. I am expecting the reported errors to be the same (except for the ignored files) and this knowledge only to be used for performance optimizations. If your rule intends to behave differently in the presence of ignored files, please use `Review.Test.dontTestWithIgnoredFiles` (TODO explanation) in your tests.

jfmengels / elm-review