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

Implement hs-bin@repl test as go bazel test #1645

Merged
merged 29 commits into from
Feb 8, 2022
Merged

Conversation

k1nkreet
Copy link
Contributor

According to #951 this PR makes a first step to implement integration tests as go_bazel_test from rules_go

@k1nkreet k1nkreet requested a review from aherrmann November 30, 2021 12:29
Comment on lines 7 to 10
native.config_setting(
name = "nixpkgs",
values = {"define": "nixpkgs=true"},
)
Copy link
Member

Choose a reason for hiding this comment

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

I think you could reuse this config_setting based on the support_nix platform constraint. Then you also don't need to introduce the --define=nixpkgs above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed

@k1nkreet
Copy link
Contributor Author

According to last ci task on Windows there is no bazel binary in PATH occurred inside bazel test, so it probably makes sense to implement an external dependency of integration_test rule on bazel binary within this PR

Ilya Polyakovskiy added 3 commits December 21, 2021 20:43
…external

  dependencies
* make integration_testing depends on external bazel binary
* bring bazel functions from rules_go and rework them to use provided
  bazel binary
@k1nkreet k1nkreet force-pushed the hsbin-as-go-bazel-test branch from d1d5c20 to 3661a87 Compare January 13, 2022 16:16
Ilya Polyakovskiy added 2 commits January 14, 2022 01:48
configurations except windows, use same bazelrc with different
configurations for tests
@k1nkreet
Copy link
Contributor Author

k1nkreet commented Jan 14, 2022

All CI tests are finally passed but I have a strong feeling that using tmp filesystem for bazel cache inside integration tests is not a good idea. Cache size will constantly grow and tmpfs on average machine is pretty limited. I've already experienced freezing of my laptop sometimes when I'm running these tests. So I will try to find another place writable by bazel tests and invariant between tests

@k1nkreet
Copy link
Contributor Author

For basic rules_haskell's integration Bazel test creates about 800MB of cache. It nearly exceeds size of $TMPDIR folder created by Nix on my machine for example but fits /tmp without any problem. So I see two ways to store cache of internal bazel:

  • Use golang os.TempDir() to pass it as $HOME for test: it uses $TMPDIR on Unix system and %TEMP% on Windows. In this case we'll need to add TMPDIR=/tmp as shellHook in shell.nix for nixpkgs configurations. Also Bazel cache will stuck in /tmp after tests run which concerns me a bit.
  • Pass $HOME through the --action_env: it will share cache between external and internal Bazel which probably could even speed up things sometimes but also breaks sandboxing of tests a bit more. Also it could affect sandboxing of other tests since they all will have non-sandboxed $HOME. Also it worth mentioning go_bazel_tests in rules_go just use common $HOME but would break with strict action_env.

I'm leaning towards the first option, @aherrmann do you have an opinion?

@aherrmann
Copy link
Member

I'm leaning towards the first option, @aherrmann do you have an opinion?

Yes, the first option seems preferable. I'd prefer to avoid the need for --action_env because it would affect all targets and impact their cache keys.

@k1nkreet
Copy link
Contributor Author

@aherrmann It's finally ready for being reviewed :)

Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

🎉 Good work, thank you!
A couple comments/questions.

WORKSPACE Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
tests/integration_testing.go Outdated Show resolved Hide resolved
tests/integration_testing.go Outdated Show resolved Hide resolved
tests/integration_tests.bzl Outdated Show resolved Hide resolved
Ilya Polyakovskiy added 5 commits January 27, 2022 20:29
  * since nested selects is not allowed add target for bindist bazel
selected by os and target for bazel selecting nixpkgs or bindist target
  * make bazel binary filegroup parameter of integration_test macro
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

This is great, thank you!
I've tested it locally and tests work as expected with and without Nix.

@aherrmann aherrmann added the merge-queue merge on green CI label Feb 8, 2022
@mergify mergify bot merged commit e2a74e5 into master Feb 8, 2022
@mergify mergify bot deleted the hsbin-as-go-bazel-test branch February 8, 2022 17:50
@mergify mergify bot removed the merge-queue merge on green CI label Feb 8, 2022
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.

2 participants