API for writing tests for lints

[typesanitizer]

I'm working on a repo so that we (the Haskell community) can share lints for commonly used packages. I'm hoping to announce it on Reddit etc. once I've solved https://github.com/theindigamer/hlint-roller/issues/1 and https://github.com/theindigamer/hlint-roller/issues/2 .

To make sure that the lints work, I'd like to write tests (issue 1). The thing is, I don't want to duplicate a bunch of logic which you already have here, for example all the stuff needed to run the tests under data/hlint.yaml.

Would it be possible to expose a small API for writing tests for lints? If yes, I'm happy to contribute a PR if you can provide guidance on what would need to be changed.

[ndmitchell]

You are meant to be able to do hlint test --hint=myhints.yaml to run the hints at the bottom of the file just hlint hlint.yaml. I haven't tested this in ages, but give it a go, and if you find any flaws we can figure out how to fix them to meet your needs.

[typesanitizer]

Okay, it did work correctly! Awesome. There are a couple of minor problems:

• The test subcommand requires that a .hlint.yaml file be present in the directory, which I didn't expect. However, this isn't an issue in practice.
• It seems that I need to duplicate imports for every test; is that correct? This is what is being done in data/hlint.yaml but it seems a bit painful in practice because I end up writing stuff like:

# import qualified Data.Map as Map \
# import qualified Data.Set as Set \
# yes = Set.fromList (Map.keys m) -- Map.keysSet m
# import qualified Data.Map as Map \
# yes = fmap fst (Map.toList x) -- Map.keys x

I'm planning to write several tests combining stuff across modules and it seems like I'll end up writing more imports than test code :sweat_smile: ...

One of the rules files can be found here.

[ndmitchell]

I fixed the issue with .hlint.yaml - nice and easy.

Yes, you need to duplicate imports, by design. The reason is that different tests will want different sets of imports - but I can see why it's a hassle for you. I guess if I'm honest I probably wouldn't do overly much testing of the rules you are adding - the rule pretty much speaks for itself - and any misunderstanding that breaks the rule is likely to be reflected in the test. I don't add new tests with new rules in hlint.yaml.

[typesanitizer]

I guess if I'm honest I probably wouldn't do overly much testing of the rules you are adding - the rule pretty much speaks for itself - and any misunderstanding that breaks the rule is likely to be reflected in the test

I agree that the examples I've shown here are a bit silly. However, once I add more stuff, I don't want to accidentally have lints that don't work ... it would be very hard to track those down if there's a mistake as users may not even know that there is a lint for X and I'm not going to be using all the packages on Hackage :).

I understand that you may not have the time to implement support for an alternate system for imports, but if you are open to the suggestion, let me know and I'll try to submit a PR in a few days.

Closing this issue for now. Thanks!

[ndmitchell]

I guess I'm curious what mistake you envisage making in the hint and how the test will catch it? That might direct what features are needed.

[typesanitizer]

Hmm, here's a complex example that I just tried out:

- warn: {rhs: "(MapS.lookup k m, MapS.insertWithKey f k x m)", lhs: MapS.insertLookupWithKey f k x m, name: Use insertLookupWithKey}
# import qualified Data.Map.Strict as MapS \
# yes = (MapS.lookup k m, MapS.insertWithKey f k x m) -- MapS.insertLookupWithKey f k x m

I wasn't certain that the double-matching on k was going to work. Surprisingly, the test didn't pass. After about a minute or so, I realized that I got the lhs and rhs messed up. Duh.

Then I tried:

# import qualified Data.Map.Strict as MapS \
# no = (MapS.lookup k1 m, MapS.insertWithKey f k2 x m)

and it did work as expected.

I can't come with a better example off the top of my head right now but I recall writing a lint for some optparse-applicative code because I'd made a mistake with using one combinator. I thought "hmm, this is perfect, we'll never have this mistake again, awesome!". Well, I made the same mistake a week later. Then I got deja-vu and went back to check the lint, and finally ended up pushing a "Fix incorrect lint" commit.

After that, I've felt wary of writing lints without small test cases. The thing is, even if something in your application blows up, you may go and ask "hey why didn't our test suite catch this earlier" but you're probably not going to your linter and check whether there was a lint for it that may not have fired.

Anyways, sorry for the rambling story, it isn't really a big deal. The big deal is being able to run the tests easily if I want to and that does work without hiccups :D.

[ndmitchell]

It feels very wrong to dissuade someone from tests... Perhaps some special syntax in an annotation should lead to that portion being copied into all subsequent annotations in that file? Perhaps \ at the end (rather than ) should be continue and accumulate? What do you suggest?

[typesanitizer]

Perhaps \ at the end (rather than )

I don't quite understand. Could you give an example?

Perhaps something like the following?

# <TEST_WITH_SHARED_IMPORTS>
# import Foo
# import Bar
# yes = ... -- ...
# no = ... -- ...
# </TEST_WITH_SHARED_IMPORTS>

[ndmitchell]

OK, I prefer your idea. How about:

# <TEST_PREFIX>
# import Foo
# import Bar
# </TEST_PREFIX>
# <TEST>
# yes = ...
# no = ...
# </TEST>

With the logic that everything in TEST_PREFIX gets prepended as a prefix to everything in TEST for the rest of that file. If you want to change the prefix, you just declare another TEST_PREFIX block and that removes the existing one. Seems sensible - does that work you? Note you may also want to turn on extensions in the PREFIX, or setup representative other data, although I imagine imports is the common setup.

[typesanitizer]

Yep, that works as I'd expect.

[ndmitchell]

Do you want to hack up the code? If not, I'll try and get round to it.

[typesanitizer]

I'll send a PR in a few days (it isn't urgent), working on other stuff ATM.