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

rust: add rust_test_binary rule #316

Merged
merged 2 commits into from
May 14, 2020

Conversation

tommilligan
Copy link
Contributor

@tommilligan tommilligan commented May 11, 2020

I have a case where I would like to build a rust binary exactly as output by rust_test, but not call it directly. Instead, I have a sh_test rule that will be the entrypoint for the test, acting as a test runner and setting up environment variables required by the test code.

This already works (amazing!), but I don't want to run the binary output by rust_test directly, as it will fail without the setup performed in the sh_test rule. This happens when running all tests with:

bazel test //...

In this PR, I have added the new rule rust_test_binary which is simply the definition of rust_test with test = True removed, and docs updated.

If there's a way to make this an argument of the existing rust_test rule please let me know, but I couldn't find any examples of how I might set the value of test on a rule dynamically.

@damienmg
Copy link
Collaborator

Hi,

Thanks for sending this PR!

I would like to figure out a better way to do it. I am not fond of the rust_test_binary API.

  • Can you explain a bit more your use case?
  • Can --run-under be used?
  • Maybe we could modify rust_test to actually include a test runner?
  • If we want to stick with the rust_test_binary could we rather make it an attribute of rust_binary?

Thanks!

@tommilligan
Copy link
Contributor Author

Good suggestions - I've had a bit of a dig, these are my initial reactions:

  • The specific use case is I have a rust library that does direct interop with an installed python package during tests. My current test rule does the following:
    • rely on the python libraries as data dependencies
    • manually configure python path, so that the python interpreter invoked finds the packages
rust_test_binary(
    name = "my_test_bin",
    crate = ":lib", # resolves to a rust_library
)

sh_test(
    name = "my_test_runner",
    # The source code of this runner does other things, but the main one is:
    # 
    # # setup environment
    # PYTHONPATH="$PYTHONPATH:$python_library_path"
    # export PYTHONPATH
    #
    # # run rust test binary
    # "$BINARY"
    srcs = ["test_runner.sh"],
    data = ["my_test_bin", "//some/other:python_lib"],
    args = [
        "--binary",
        "$(location :my_test_bin)",  # path of the rust binary to test
        "--python-path",
        "some/other"                 # value which will be set on PYTHONPATH
    ],
)
  • --run_under is cool - I wasn't aware of it, and I can get the specific test case I gave to pass using it. However, this isn't configurable on a per-test basis and so isn't suitable here.
  • I'm not quite sure how bundling a test runner into rust_test is better than allowing users to write their own test runner (which is essentially what sh_test is an escape hatch into). I think this is an edge case that most users won't need, and equally I'd like the flexibility to define my own runner in a way that isn't tied to rules_rust.
  • having test = True set on the rust_test rule happens outside the _rust_test_impl function, and I haven't seen any examples of it being set dynamically, so I don't think this is feasible.

I agree that I initially didn't like having a separate rule, but I think it makes sense for a couple of reasons:

  • rust_test currently incorporates two actions from my perspective:
    • building a rust executable, with test features enabled
    • marking this as a bazel test
  • I also continued to investigate whether I could replicate the same behaviour just using sh_test depending on a rust_binary rule, with rustc_flags = ["--test"]. But I didn't manage to, and it significantly increases boilerplate and duplication between the rust_test and rust_library rules.

@damienmg
Copy link
Collaborator

Ok so from your description it really sounds like an edge case.

What do you think about making rust_test_common public and so you can define that rust_test_binary in your own repository? This way we avoid exposing an API that does not feel totally right at the moment.

@tommilligan
Copy link
Contributor Author

That sounds good in principle - the only snag I can see is that rust_test also depends on _rust_common_attrs and _rust_test_attrs, so I'd want them to be public in the same way. If not, I'd still end up depending on the private API, which doesn't seem worth it.

I agree that this is an edge case - this is why the solution I've proposed doesn't relate to the details of it in any way. It just provides an entrypoint for the user to add their own test runner.

Could you clarify why expecially you don't like the rust_test_binary API? If you're concerned with supporting a larger public interface, It seems to me that exposing rust_test_common is actually a larger API surface to publicly maintain.

@damienmg
Copy link
Collaborator

You are correct, exposing *attrs feels wrong too. My view is that rust_test_binary is a new rule that does not distinguish itself enough from rust_binary.

Since I cannot find any good way to make it better, could we just replace the documentation by a warning that this is experimental?

@tommilligan
Copy link
Contributor Author

Happy to do so - I have updated the docstring.

I have been trying to find a generic solution for other test targets that produce executable output, but from reading the Bazel docs, it seems test rules really are a special case all of their own. You even have to ensure that the rule name is *_test and you can't name non-test rules with this suffix. So it seems that parametrizing such a rule is not possible at the native level.

I've just noticed that the HTML documentation is not autogenerated - I'm happy to add an entry in the docs, but from the sound of it you may prefer to leave it undocumented for now?

@damienmg
Copy link
Collaborator

Correct. Thanks for the update!

@damienmg damienmg merged commit 619fcab into bazelbuild:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants