Move as much of tests/RunTests.hs into bazel, remove if possible

[Profpatsch]

For adding our repo to the upstream integration CI (#754, bazelbuild/continuous-integration#235) we need to pull many/all of our tests into bazel rules.

I’m pretty certain many of these can be implemented as simple sh_tests (or genrules) instead.

[guibou]

What's provided as a build context ? Is the builder blindly runs bazel test //... or can it be configured to call multiples bazel commands?

[Profpatsch]

What's provided as a build context ?

I don’t understand that question :)

Is the builder blindly runs bazel test //... or can it be configured to call multiples bazel commands?

Ideally, bazel test //... should do what it says on the tin, run every test. If we cannot represent some tests within bazel (or compile in different modes), that is a bug in bazel in my book.

[guibou]

@Profpatsch

I was wondering how our code will be tested on the upstream integration CI? What control do we have on how it will be tested? What interface should we provide.

As you correctly said, most of our tests can be rewrited as genrule or sh_test, but a few of them needs more control on the options we pass to bazel. If I remember well, some of them need the result of bazel query before calling a few bazel command. Some others needs some custom bazel flags (such as -c dbg).

To understand how / what we must rewrite for the upstream CI system, we need to know how it behaves.

Ideally, bazel test //... should do what it says on the tin, run every test. If we cannot represent some tests within bazel (or compile in different modes), that is a bug in bazel in my book.

There is a lot of limitations in bazel which are not bug, but just limitations due to the fact that this project is not finished.

[thufschmitt]

IIRC a relatively common practice is to run bazel whithin bazel test (see for example https://github.com/bazelbuild/bazel-integration-testing)

[guibou]

Actually, just having bazel test //... part of bazel CI is a great improvement. We can iterate later on the removal of the test runner, if necessary.

[Profpatsch]

Actually, just having bazel test //... part of bazel CI is a great improvement. We can iterate later on the removal of the test runner, if necessary.

You should add that comment to the upstream issue.

[guibou]

Which one?

[Profpatsch]

bazelbuild/continuous-integration#235

[aherrmann]

bazel test //tests/... covers most tests in rules_haskell. However, there are a few tests that still require //tests:run-tests, e.g. failure tests, REPL tests, pinning tests...

rules_go has go_bazel_test to run bazel inside tests and thereby allows to define integration tests and failure tests. According to bazelbuild/bazel-integration-testing#170 these are considered superior to https://github.com/bazelbuild/bazel-integration-testing. E.g. coverage tests are implemented using go_bazel_test.

rules_haskell could implement its own version of go_bazel_test, say haskell_bazel_test, and we could express the remaining tests in //tests:run-tests using haskell_bazel_test. With that bazel test //... would cover all tests.

[k1nkreet]

I will try to carefully suggest and gradually implement sequential steps to resolve this issue.

Here is the list of tasks presented in RunTests.hs:

• "bazel lint" aka bazel run //:buildifier: There is a buildifier_test-rule in Bazel's buildtools repository which can easily replace it as a Bazel test. Unlike buildifier-rule operated on workspace it demands sources list, so there should be created a filegroup with all rules_haskell's sources (which is also necessary for go_bazel_test) There is a WIP PR by @tanyabouman adding buildifier_test along with gazelle extension for creating filegroups of specific filetype and filegroups created with this extension. I suggest to keep this things orthogonal: manually create this filegroup, use it in buildifier_test and go_bazel_test-like rules and create a gazelle extension separately to simplify the maintenance of this filegroup.
• "bazel test" aka bazel test //...: will be removed as a last step when everything will be replaced as a Bazel tests
• "bazel test prof": runs bazel test with -c dbg and additional test_tag_filters. I don't see a good way to include it in bazel test //... at the moment.
• "bazel build worker" aka bazel build //tools/worker:bin: I don't understand it's appearance here: bazel test //... triggers build of this target perfectly unless --build_tests_only specified. Maybe I've missed something and @aherrmann can clarify this.
• "stack_snapshot pinning" and "repl tests" are perfectly applicable for go_bazel_test's analog.

I will try to elaborate a bit on how go_bazel_test works and what analog I can suggest. It creates a go_library for:

• Creating persistent location in execroot
• Linking rules_go sources over there (same target I described in buildifier part)
• Creating specified go projects
• Running some Bazel command over there

go_bazel_test is a go_test which can use this library to prepare workspace, run Bazel and examine outputs. For me it's a bit odd to keep this functionality language specific for Haskell. The task of preparing directory, creating some files, run arbitrary command and examining it's output sounds as a shell script. So I would suggest to implement sh_library with same functions and analogous sh_test.

So I will try to create a PR's for every step consequentially.

[aherrmann]

@k1nkreet thank you for the structured outline!

• I suggest to keep this things orthogonal: manually create this filegroup, use it in buildifier_test and go_bazel_test-like rules and create a gazelle extension separately to simplify the maintenance of this filegroup.

Yes, I agree that keeping this orthogonal is a good approach!

• "bazel test prof": runs bazel test with -c dbg and additional test_tag_filters. I don't see a good way to include it in bazel test //... at the moment.

Good question, perhaps we could use transitions to create versions of tests in other configurations such as dbg.

• "bazel build worker" aka bazel build //tools/worker:bin: I don't understand it's appearance here: bazel test //... triggers build of this target perfectly unless --build_tests_only specified. Maybe I've missed something and @aherrmann can clarify this.

Good catch. I think this one is an oversight from #1569. Before that RunTests only built with --build_tests_only and the bindists only tested bazel test //tests/..., so the worker was not covered. Since #1569 this should no longer be needed.

[aherrmann]

Another alternative for bazel-in-bazel integration testing next to rules_go and bazel-integration-testing is rules_bazel_integration_test.
Relatedly, there is rules_bzlformat which offers an alternative for buildifier based linting with convenient macros to generate the needed filegroups.

[k1nkreet]

Finally #1766 appeared at master. There is a mixed approached introduced in this PR:

1. A rules_haskell_integration_test macro is introduced which is macro on top of rules_bazel_integration_test so it is supported creation of test workspaces inside rules_haskell's workspace (instead of encoding test workspace in specifically formatted string like go_bazel_test does)
2. From rules_bazel_integration_test it inherits ability to use multiple versions of bazel binary distributions for testing
3. Also supports multiple bazel nixpkgs distributions
4. This macro implements the same way of execution as go_bazel_test does to provide caching between tests speeding them up significantly
5. Test scenarios for rules_haskell_integration_test should be written in Haskell
6. All existed integration_tests based on go_bazel_test- implementation reworked with rules_haskell_inegration_test

There is a plenty of tests in RunTests.hs which should be gradually transformed into rules_haskell_integration_tests