separate tests into `lens-test` repository about lens HOT 13 CLOSED

When @jackfirth and I discussed this before, we had a preference for the tests being in the same file as the stuff it was testing, in a module+ test submodule, because that forces it to be in build-deps, but not in deps. At the time we both thought that was good enough.

from lens.

I think everything would remain the same, other than moving the tests to different files. The tests would remain in a module+ test and the depository would only have build-deps.

from lens.

Here is a sample of how things might be reorganized: #298

from lens.

I dislike tests in separate files because (1) I lose the ability to immediately and automatically run them with the file they test, (2) I lose the ability to test non-public features without require/expose which makes it less clear what private code may break tests when refactoring, and (3) it's harder to find the tests for a given file. I think keeping test code in build-deps is good enough, because the built packages provided by the package build server can be installed without build dependencies.

Can you give more details about why avoiding this coupling would help? A normal client of the lens library would likely want both the -common package and the -data package, the only libraries I can think of that want just the -common package would be extensions to lens like collections-lens that provide their own lenses.

from lens.

A quick note in case we do decide on a lens-test package: you can use compile-omit-paths in a collection's info to avoid triggering runtime dependencies on rackunit even when the test file doesn't put its checks in module+ test.

from lens.

Can you give more details about why avoiding this coupling would help?

Right, what if I'm defining my own lenses? Isn't that why -common and -data are separate in the first place?

(1) is convenient but don't programmers typically run raco test before committing anyways? Any then ci will run tests as well. I actually tend to dislike (1) because if the tests are decentralized, then I'm never sure if I'm running them all.

(2) is a problem though I haven't seen it so far in this package. Maybe it wouldnt be a problem to leave those tests in place?

(3) is annoying but if the same directory structure is maintained, which is what other packages do, then is the annoyance reduced somewhat?

from lens.

When defining your own lenses, you'd have a runtime dependency on lens-common. But when anyone uses your lenses, they're probably using them along with other lenses in lens-data. And if they aren't, installing with --binary or --binary-lib means you won't pull in build dependencies anyway.

I agree it's pretty common to run raco test before committing, and I do that in addition to running tests in the current file. The current file runs are still very useful to me because it's a near instant feedback loop that I don't have to configure anything to get - no fiddling with command line flags and options, no messing with editor configuration, and I don't even have to remember to enable it. And because it's limited to the current file it's very fast. I don't have to push commits and wait several minutes for Travis to report test failures because I forgot to run raco test. It's such a smooth experience that I'd want a test package split to provide a lot value before giving up same-file tests.

If there are specific packages that really, really need to avoid pulling in -data and can't rely on the build server, I'd want to know what they are to better understand what value this brings.

from lens.

Ok, just wanted to bring it up as a possibility. If you guys already discussed it then I respect whatever you decide. Thanks.

from lens.

These are my thoughts, I'm not sure what @AlexKnauth's position on it is. Since I'm personally not in favor of adding the -test package and you are, I'll defer to @AlexKnauth as a tie breaker :)

from lens.

I agree with @jackfirth; Having tests in same file is worth it.

One possible (but hacky) best-of-both solution would be to add a macro around the tests, to run them only when lens/data/... is available and dynamic-require-ing from it succeeds. Does that sound like something we should do, or is that too much complexity for getting rid of one thing in the build-deps?

from lens.

That could work, but I don't think it's worth the effort. It's certainly not code I'd want to maintain and interact with every time I write a unit test.

from lens.

Ok, thanks for discussing the issue.

from lens.

Thank you for broaching the subject so amicably @stchang!

from lens.