Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move tests to rules_haskell_test module #1903

Merged
merged 7 commits into from
Jun 9, 2023
Merged

Conversation

ylecornec
Copy link
Member

@ylecornec ylecornec commented May 26, 2023

To prepare for bzlmod support, this PR moves most tests from rules_haskell to a dedicated module called rules_haskell_tests.

Testing rules_haskell from another module is more realistic (which is particularly important with bzlmod since different modules have different visibility and repository mappings). In addition, once bzlmod is activated, the rules_haskell_test module will be able to depend on the rules_haskell_nix module which will be used to register the the nix toolchains (to avoid a dependency between rules_haskell and rules_nixpkgs).

Some comments

  • Some targets are still present in rules_haskell's test folder for tests that do not test the public api but are internal to the module such as the shellcheck ones or //tests/package_configuration:package_configuration_test.

  • Dependencies from the WORKSPACE and the non_module_deps_* files are split between the two modules. And the dependencies from non_module_dev_dep_* files will be development dependencies with bzlmod.

About the CI

  • The CI was updated so that tests for rules_haskell and rules_haskell_tests happen in parallel. So there are more jobs but the tests for the rules_haskell module should be quick since most tests are now in rules_haskell_tests.

  • The config was also updated so that rules_haskell and rules_haskell_test use separate caches on windows. This works around undeclared inclusions issues such as this one.
    This can happen because the *.d files (that keep track of C dependencies for hermeticity purposes) such as the one file from this cache hit contain absolute paths that are not the same for both module and should not be shared.

  • The mergify configuration was updated but I think this is not effective until the PR is merged, so it will have to be merged manually.

@ylecornec ylecornec force-pushed the ylecornec/rules_haskell_test branch from 57041bd to 006e47c Compare May 30, 2023 08:36
@ylecornec ylecornec force-pushed the ylecornec/rules_haskell_test branch 6 times, most recently from 58bf806 to 2f3635b Compare May 31, 2023 09:42
@ylecornec ylecornec marked this pull request as ready for review May 31, 2023 11:15
@ylecornec ylecornec requested a review from avdv as a code owner May 31, 2023 11:15
Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, LGTM!

Could you please adjust the README to include a few words on how to run tests now?

BTW, for rules_sh we added the test module (as a local repository) to the root WORKSPACE too, so you could run all tests with bazel test //... @rules_sh_tests//.... Maybe this could be done here too, making it a bit easier to run the tests? What do you think?

@aherrmann
Copy link
Member

BTW, for rules_sh we added the test module (as a local repository) to the root WORKSPACE too, so you could run all tests with bazel test //... @rules_sh_tests//.... Maybe this could be done here too, making it a bit easier to run the tests? What do you think?

I would not spend much effort in that direction. WORKSPACE are not loaded transitively, so testing bazel test @rules_sh_test//... is quite different from cd test && bazel test .... It's also not applicable to the bzlmod world since library modules are not allowed to use local overrides.

@ylecornec ylecornec added the merge-queue merge on green CI label Jun 9, 2023
@avdv avdv merged commit c88f2e4 into master Jun 9, 2023
@avdv avdv deleted the ylecornec/rules_haskell_test branch June 9, 2023 15:11
@mergify mergify bot removed the merge-queue merge on green CI label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants