rules_haskell
rules_haskell copied to clipboard
Move as much of tests/RunTests.hs into bazel, remove if possible
For adding our repo to the upstream integration CI (https://github.com/tweag/rules_haskell/issues/754, https://github.com/bazelbuild/continuous-integration/issues/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_test
s (or genrule
s) instead.
What's provided as a build context ? Is the builder blindly runs bazel test //...
or can it be configured to call multiples bazel commands?
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.
@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.
IIRC a relatively common practice is to run bazel whithin bazel test (see for example https://github.com/bazelbuild/bazel-integration-testing)
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.
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.
Which one?
https://github.com/bazelbuild/continuous-integration/issues/235
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 https://github.com/bazelbuild/bazel-integration-testing/pull/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.
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 abuildifier_test
-rule in Bazel's buildtools repository which can easily replace it as a Bazel test. Unlikebuildifier
-rule operated on workspace it demands sources list, so there should be created a filegroup with allrules_haskell
's sources (which is also necessary forgo_bazel_test
) There is a WIP PR by @tanyabouman addingbuildifier_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 inbuildifier_test
andgo_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 inbazel 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 inbuildifier
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.
@k1nkreet thank you for the structured outline!
- I suggest to keep this things orthogonal: manually create this filegroup, use it in
buildifier_test
andgo_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 inbazel 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 https://github.com/tweag/rules_haskell/pull/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.
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 filegroup
s.
Finally #1766 appeared at master. There is a mixed approached introduced in this PR:
- 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 insiderules_haskell
's workspace (instead of encoding test workspace in specifically formatted string likego_bazel_test
does) - From
rules_bazel_integration_test
it inherits ability to use multiple versions ofbazel
binary distributions for testing - Also supports multiple
bazel
nixpkgs distributions - This macro implements the same way of execution as
go_bazel_test
does to provide caching between tests speeding them up significantly - Test scenarios for
rules_haskell_integration_test
should be written in Haskell - All existed
integration_tests
based ongo_bazel_test
- implementation reworked withrules_haskell_inegration_test
There is a plenty of tests in RunTests.hs
which should be gradually transformed into rules_haskell_integration_test
s