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

Activating features for tests/benchmarks #2911

Open
ctz opened this issue Jul 24, 2016 · 55 comments
Open

Activating features for tests/benchmarks #2911

ctz opened this issue Jul 24, 2016 · 55 comments
Labels
A-features Area: features — conditional compilation S-propose-close Status: A team member has nominated this for closing, pending further input from the team S-triage Status: This issue is waiting on initial triage.

Comments

@ctz
Copy link
Contributor

ctz commented Jul 24, 2016

Given a library crate, I want to have optional features which are available to integration-level tests and examples so that 'cargo test' tests them.

Here's what I have:

[package]
name = "rustls"
version = "0.1.0"
(etc)

[features]
default = []
clientauth = []

[dependencies]
untrusted = "0.2.0"
(etc)

[dev-dependencies]
env_logger = "0.3.3"
(etc)

Here's what I've tried so far. Attempt one: add the crate into dev-dependencies with the feature, ie:

[dev-dependencies.rustls]
path = "."
version = "0.1.0"
features = ["clientauth"]

I kind of expected that to work, but it panics:

jbp@debian:~/rustls$ RUST_BACKTRACE=1 cargo test
thread '<main>' panicked at 'assertion failed: my_dependencies.insert(dep.clone())', src/cargo/util/dependency_queue.rs:86
stack backtrace:
   1:     0x558d17c3a2a0 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
   2:     0x558d17c4383b - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
   3:     0x558d17c434c3 - std::panicking::default_hook::hc2c969e7453d080c
   4:     0x558d17c28048 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
   5:     0x558d17701fef - std::panicking::begin_panic::h4ebf9fe884b2415f
   6:     0x558d17802d27 - cargo::ops::cargo_rustc::job_queue::JobQueue::enqueue::h98b20a33b8744ebe
   7:     0x558d177f4faf - cargo::ops::cargo_rustc::compile::h93336dd21aba29d0
   8:     0x558d1777a513 - cargo::ops::cargo_rustc::compile_targets::h7223dffae01d6b9d
   9:     0x558d1776eec9 - cargo::ops::cargo_compile::compile_pkg::h48656ffa9b1541f3
  10:     0x558d1776bd09 - cargo::ops::cargo_compile::compile::h1b43b20047c53d10
  11:     0x558d1785fb0d - cargo::ops::cargo_test::compile_tests::h028a22d3131e0d65
  12:     0x558d1785f379 - cargo::ops::cargo_test::run_tests::h0ad98c54a6f40c9d
  13:     0x558d176de256 - cargo::call_main_without_stdin::hd484f08b17c419d1
  14:     0x558d1768afe7 - cargo::execute::hab7accd6bf9b5c64
  15:     0x558d17683f8b - cargo::call_main_without_stdin::hf099bd36acb849a3
  16:     0x558d17680da1 - cargo::main::h2b219a79f378c1ef
  17:     0x558d17c430d8 - std::panicking::try::call::hc5e1f5b484ec7f0e
  18:     0x558d17c4e0eb - __rust_try
  19:     0x558d17c4e08e - __rust_maybe_catch_panic
  20:     0x558d17c42b03 - std::rt::lang_start::h61f4934e780b4dfc
  21:     0x7f79ea9bf86f - __libc_start_main
  22:     0x558d17680728 - <unknown>

(version: "cargo 0.11.0-nightly (259324c 2016-05-20)")

Attempt two: see if features.* works like profile.*:

[features.test]
default = ["clientauth"]

alas

$ cargo test
error: failed to parse manifest at `/home/jbp/rustls/Cargo.toml`

Caused by:
  expected a value of type `array`, but found a value of type `table` for the key `features.test`

Is this possible?

See also #4663

@alexcrichton
Copy link
Member

Unfortunately, no, it's not possible for cargo test to automatically enable features currently. The panic though definitely seems bad!

@alexcrichton alexcrichton added the A-diagnostics Area: Error and warning messages generated by Cargo itself. label Jul 25, 2016
@jjpe
Copy link

jjpe commented Dec 18, 2016

I just ran into this issue because I'm currently using a feature to decide whether or not to do some rather verbose tracing. I'd like to be able to set this during testing so that when tests fail I can look at the trace.
The reason I made it a feature is that the code itself is in a library, but at the same time it seems more appropriate to have the consuming crate decide whether or not to perform the trace.

To be clear, an actual consuming crate has no problem setting the feature and getting corresponding results. It is just for testing the library crate itself that it doesn't seem possible at the moment.

Update: There is a workaround I just found:
If the feature is called foo, then this will test with the feature activated:

cargo test --features "foo"

The only thing is that this is activated manually, and being able to specify it in the library's Cargo.toml would activate the feature by default, which is desirable in my use case.

@sanmai-NL
Copy link

sanmai-NL commented May 4, 2017

@alexcrichton:

As of yet, the Cargo panic is gone.

  cargo --version
cargo 0.19.0-nightly (fa7584c14 2017-04-26)

Excerpt from Cargo.toml

[features]
default = [] # "clippy"

[profile.test]
default = ["clippy"]

The output is now as follows:

cargo test --color=always --package my_app --bin my_app test_1 -- --nocapture
warning: unused manifest key: profile.test.default
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/deps/structure_web_server-3dc5f86e9b4fb259

running 1 test
test tests::test_1 ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured

You wrote this isn't possible. It appears there are no immediate objections to adding such a feature. Could you explain how fundamental the limitation is? And, if possible, sketch a solution? Just so a PR becomes more likely to happen.

@alexcrichton
Copy link
Member

Ah I believe this was fixed in #3921 as #3902 was a dupe of this issue. I guess I was wrong before!

@sanmai-NL
Copy link

sanmai-NL commented May 4, 2017

@alexcrichton: the crashes have been resolved yes, but the original feature request still stands AFAIK. Please reopen.

@alexcrichton
Copy link
Member

Oh sorry, missed that aspect.

@sunjay
Copy link
Member

sunjay commented Oct 27, 2017

Is there any way to add features per profile now? That is, if I want to enable certain features during tests, can I do that yet?

@alexcrichton
Copy link
Member

@sunjay unfortunately no, you'll still need to pass --features on the command line

@sunjay
Copy link
Member

sunjay commented Oct 28, 2017

Is this change something that would require an RFC or could it just be implemented? It sounds like being able to specify features in the profile configuration is something that could benefit a lot of people.

@alexcrichton
Copy link
Member

@sunjay I think due to the subtle possible ramifications of auto-enabling features I'd personally want to see an RFC, but it may be worth canvassing others from @rust-lang/cargo first as well

@sunjay
Copy link
Member

sunjay commented Oct 30, 2017

I would love to work on an RFC for this. Very interested to hear what the cargo team thinks.

Right now, I'm having to constantly remember to pass features in when I run cargo test. I would really prefer to not have to do that. It's an extra point of friction for anyone who uses my code.

Though it would be great to even just be able to configure features in tests, I would really like to see cargo support a features/default features option in each of the [profile.*] sections. This would make it so you could configure default features for your release, debug and benchmarking profiles too. My RFC would be proposing something like that.

@matklad
Copy link
Member

matklad commented Oct 31, 2017

I think we have an RFC by @withoutboats for this already: rust-lang/rfcs#1956, which is postponed because of profiles.

@sunjay
Copy link
Member

sunjay commented Oct 31, 2017

@matklad Ah yup that's exactly what I would have proposed. Thanks for looking that up! I read the discussion and it seems like that's pending on some work with how multiple profiles are applied together (e.g. test --release). It seems that the current behavior is more complicated than what it looks like at first glance.

Is there any way to do this in a backwards compatible way? It would be great if there was some way to just add another configuration option that is in effect the same as the command line argument while keeping everything the same as it is now. Would that make it too hard to change to something better (like profiles) later?

The main issue I'd like to solve is that right now, introducing features into your codebase makes it so that you have to pass features in as command line arguments every time you run a certain commands. If these aren't default features that you need for all builds, it's not possible to simply configure the ones you want in Cargo.toml. This makes it harder for people new to your codebase or to cargo set things up. It is very repetitive and easy to forget. Is there an easier way to just address that issue?

@brson
Copy link
Contributor

brson commented May 21, 2018

I've also run into the need to have features automatically enabled in test builds. In my case I want the python bindings "extension-module" feature on for production builds, but off for test builds so that I can link directly to python during testing.

@Object905
Copy link

Also it's very useful for #![no_std] ergonomics. Since you need to test with libstd linked, otherwise you get
error: no #[default_lib_allocator] found but one is required; is libstd not linked?. So you constantly running cargo test --features std

@ebfull
Copy link

ebfull commented Jul 2, 2018

Note that you can do this with required-features if you specify your test targets explicitly, aka:

[[test]]
name = "whatever"
path = "tests/whatever.rs"
required-features = ["mycoolfeature"]

@RReverser
Copy link
Contributor

@ebfull That only skips building/running tests if feature is not enabled instead of force-enabling the feature during build.

@synek317
Copy link

synek317 commented Aug 25, 2018

Some ugly workaround, at least for some cases, is to extend all your #[cfg(feature = ...)] with test (e.g. #[cfg(any(test, feature = "..."))] and copy all optional dependencies in [dev-dependencies] and remove optional = true.

It works nice for me but for sure there are cases where it is not enough.

@nixpulvis
Copy link

Yea I feel like an RFC here would make sense if one doesn't already exist. I'm using rust again, and finding myself really wanting something like cargo test --feature-matrix for example.

@ehuss ehuss added A-features Area: features — conditional compilation and removed A-diagnostics Area: Error and warning messages generated by Cargo itself. labels Nov 18, 2018
@ilyagr
Copy link

ilyagr commented Sep 29, 2023

In case of jj, the tests use a couple of binaries for a "fake editor" (for example). We need some way to not have these binaries installed when the user runs cargo install jj-cli. The lack of fake editor in the binary releases also caused cargo binstall to refuse to install jj-cli.

In martinvonz/jj#2277, I used the dev-dependencies hack to acheive this. Unfortunately, that doesn't work well with rust-analyzer, see e.g. rust-lang/rust-analyzer#14167 . Other than that, it works. (Update: TBH, we already had issues with rust-lang/rust-analyzer#14167 before my PR; I'm not 100% sure if having to use dev-dependencies for that PR actually made the problem any worse. But now we have that problem for two reasons rather than one.)

For my purpose, I don't need any conditional compilation within a crate, only a way to prevent generating some binaries outside of tests.

@epage
Copy link
Contributor

epage commented Sep 29, 2023

Thanks @ilyagr for the clear use case!

Access to test binaries is a sore point right now I can see how this helps in that case. Another feature that could help is artifact dependencies

@martinling
Copy link

martinling commented Oct 1, 2023

Here's another use case from our GUI application Packetry.

Although our codebase does include some unit tests written in the usual way with #[cfg(test)] and #[test], the vast majority of testing is carried out by a specific test called test_replay. This test replays a large amount of input data and user actions into the UI code, and checks the results.

That test has to be declared in Cargo.toml with harness = false, because it calls into GTK, and on macOS, GTK can only be called from a program's original main thread. The standard cargo test harness does not run tests in the main thread, even if you run cargo test with -j 1. So we have to write the test as a standalone binary without the standard harness, or it won't work on macOS.

To achieve the testing we want, we have to add a bunch of logging output to our normal UI code. We don't ever want that logging code at normal runtime. It's only there for testing. But we can't gate it behind #[cfg(test)], because that isn't enabled when building our separate test binary: it only applies for tests specified in the standard way. So we have to gate the logging code behind a feature instead, which we call test-ui-replay.

Most of the above details aren't relevant: what matters is just that we have a custom test, not using the standard harness, which requires some feature, and which we want to always be run.

In Cargo.toml, we use required-features to specify that the test_replay test requires the test-ui-replay feature. But we still have to specifically request that feature, every time we test: cargo test --features=test-ui-replay

What we want is to be able to just run cargo test, and have this test always be run. It's absolutely fine that we won't be able to test without that 'feature'. That's exactly what we want. In our case it's not even a feature in the normal sense - it's only there because we can't use #[cfg(test)], which in turn is because we're not using the standard test harness for that test.

I've just tried, and the dev-dependency hack works for us. But that's not documented anywhere other than this thread, and the discussion in #9518 says that this isn't really something that should be allowed, and the only reason it's not being made an error yet is that it allows working round this issue. That doesn't sound like something we should be relying on.

I am opposed to closing this issue without either a proper solution, or another issue left open tracking what's needed.

The proposal to close suggests:

  • the self-dev-dependency hack: as above, this seems to only work by accident, and is not documented as supported.
  • test.required-features: we're already using this, it doesn't solve our problem.
  • #[cfg()] on the relevant tests/mods: we're already using this, it doesn't solve our problem.
  • Allow dependencies that only apply to specific cargo targets (bin, example, etc.) #1982: this only covers build-target-specific dependencies, not features required in the current package.
  • #3374: this RFC is closed, and there's no other current proposal.

@epage
Copy link
Contributor

epage commented Oct 2, 2023

Most of the above details aren't relevant: what matters is just that we have a custom test, not using the standard harness, which requires some feature, and which we want to always be run.

Actually it is relevant, particularly the combination of (1) tests that need access to private APIs or behaviors and (2) not being able to write these as unit tests (with enough context to understand the "why")

This kind of information is the core of whats needed to decide how important a use case is and how to handle it.

Is the logged information access through a feature-gated API or is it exposed through stdio / GUI?

@epage
Copy link
Contributor

epage commented Oct 2, 2023

@ilyagr another potential workaround is to have the test bins auto-discovered b
but to ensure they aren't included in the published artifact (.crate) via package.include or package.exclude.

@ilyagr
Copy link

ilyagr commented Oct 2, 2023

@epage

another potential workaround is to have the test bins auto-discovered b but to ensure they aren't included in the published artifact (.crate) via package.include or package.exclude.

Could you elaborate? I'm far from an expert on Cargo.toml. I'm not sure what "auto-discovered" means.

Naively, I was wondering whether there could be a dev-dependency on a separate crate for the fake binaries, but I'm not sure whether the tests will be able to find the binaries from another crate.

@martinling
Copy link

Actually it is relevant, particularly the combination of (1) tests that need access to private APIs or behaviors and (2) not being able to write these as unit tests (with enough context to understand the "why")

I take your point; I was just trying to pre-emptively avoid the discussion getting dragged into the weeds of why GTK/macOS has that particular annoying limitation, when there are any number of other reasons why someone might want to have tests that don't use the standard test harness.

Is the logged information access through a feature-gated API or is it exposed through stdio / GUI?

The test creates a UI in the same way the normal program would, but the feature causes the UI to be created off-screen, with logging code. The test then uses feature-gated APIs on the UI to set where to direct the logging, and also to inject a sequence of pre-recorded user actions into the UI. The log output is then compared against a reference file, and if the two match, the test passes.

As I understand it, the reason that #[cfg(test)] doesn't work for us is that the UI code is in a library target, and that library target is built without cfg(test) set, because a standalone test is presumed to be an integration test that only uses public APIs. In fact, we don't have any truly public API at all - we don't publish a library crate. The library target only exists so that we can share UI code between the main program and the replay test.

@martinling
Copy link

For our particular use case, if there were a way to tag a unit test as "must be run in the main thread of a process", and have Cargo honour that somehow (either by executing from its main thread or spawning a new process), then that would be a better solution for us. Originally test_replay was just a unit test in main.rs; it's only because that wouldn't work on macOS that I had to break up main.rs into lib.rs, ui.rs, and test_replay.rs so that it could run as a standalone test.

@epage
Copy link
Contributor

epage commented Oct 3, 2023

@ilyagr

another potential workaround is to have the test bins auto-discovered b but to ensure they aren't included in the published artifact (.crate) via package.include or package.exclude.

Could you elaborate? I'm far from an expert on Cargo.toml. I'm not sure what "auto-discovered" means.

By default (controlled by package.autobins), cargo will look for main.rss in specific locations and automatically create [[bin]] tables for them. This auto-discovery happens on every load. If you specify that some main.rs should not be included in the .crate file, then when a user does cargo install jj-cli, cargo won't find those additional main.rs files and won't install them.

Naively, I was wondering whether there could be a dev-dependency on a separate crate for the fake binaries, but I'm not sure whether the tests will be able to find the binaries from another crate.

That is what the artifact dependencies feature is about that I linked to. Its not just about depending on them but needing to tell cargo to build the [[bin]]s and not just the [lib].

@epage
Copy link
Contributor

epage commented Oct 3, 2023

@martinling The reason for that question I asked was to understand whether there would be problems by the fact that cargo test could only ever run your code in a non-production mode.

btw some other potential workarounds for your use case

  • Put most of your code in a lib and have another package depend on it, activating the feature that is needed, and have the tests live there.
  • Adding accessibility support (which would likely be a lot of work) as that tends to have enough overlap for testing

@martinling
Copy link

Put most of your code in a lib and have another package depend on it, activating the feature that is needed, and have the tests live there.

I have just verified that yes, it is possible to make cargo test work as desired by further splitting what was originally a simple, single-binary project into three different packages, with four Cargo.toml files. But what was the point of this suggestion?

Yes, it is possible to work around the lack of the feature that people are asking for in this issue. That doesn't make the feature request invalid. The complexity of the workarounds you're suggesting illustrate exactly why it would be helpful.

Adding accessibility support (which would likely be a lot of work) as that tends to have enough overlap for testing

Even with a lot of work, I do not believe that it would be either practical or performant for us to implement the same test through the GTK accessibility APIs. We're not just clicking on some buttons and looking for dialog messages. We're dealing with a list model with what may be millions of rows that the UI loads from on demand. And the test also needs to control the inflow of packets into the decoder engine so that it can verify how descriptions of items are updated by new packets. There's no way to do all that through the normal application UI, we have to hook into it specially, and that requires gating some code.

But even if we could, the fact that this may a suitable solution for some GUI applications doesn't invalidate the use cases where it isn't.

Why is there so much desire to close this? I don't really see anyone asking for anything complicated here. A solution could be as simple as a Cargo.toml key that makes cargo test behave as if you'd typed cargo test --features=foo,bar. As far as I can see, it doesn't need to affect how feature unification works, or any other Cargo internals. It's just about setting some defaults per-profile on the frontend side.

@epage
Copy link
Contributor

epage commented Oct 3, 2023

I have just verified that yes, it is possible to make cargo test work as desired by further splitting what was originally a simple, single-binary project into three different packages, with four Cargo.toml files. But what was the point of this suggestion?

Yes, it is possible to work around the lack of the feature that people are asking for in this issue. That doesn't make the feature request invalid. The complexity of the workarounds you're suggesting illustrate exactly why it would be helpful.

I would assume you could get away with just two packages and two Cargo.toml files (workspace + lib + main bin, test bin)

As for the point, it was to offer another alternative solution for people running into it. It might work for some people; it might not work for others. It does offer the benefit of having the minimal affect on feature activations which is likely needed in some other cases.

I do feel it important to call out that what is important in an issue is the use cases. We shouldn't get attached to any given solution.

Why is there so much desire to close this? I don't really see anyone asking for anything complicated here. A solution could be as simple as a Cargo.toml key that makes cargo test behave as if you'd typed cargo test --features=foo,bar. As far as I can see, it doesn't need to affect how feature unification works, or any other Cargo internals. It's just about setting some defaults per-profile on the frontend side.

I think there might be a misunderstanding. None of this is "arguing back to force through a closure". I am trying to understand use cases and explore the problem space. There are valid workflow problems people are having and they weren't all communicated earlier. The question is whether this is still the right solution. That, I'm still not sure on.

And yes, we can add a lot of little features like this but

  • I've found the more features you have, the less value you get out of them because you overwhelm users and they can't find what they need to solve problems. Its best to look for patterns in case you can find wider ways of solving problems. There might even be times when its appropriate to say "this is too narrow of case for built-in support so long as there are valid workarounds"
  • We need to consider the whole workflow and how the feature is used to ensure we are helping drive the right behavior, while still allowing the exception cases. Otherwise, a "simple" issue like this could turn into 10 more issues that want to extend whatever solution we come up with in 100 different ways because it doesn't quite work for them.

@martinling
Copy link

I think there might be a misunderstanding. None of this is "arguing back to force through a closure". I am trying to understand use cases and explore the problem space.

That intent would have been clearer had you not first entered the discussion with a proposal to close, and then proceeded to respond to use cases by listing workarounds.

But anyway. In our specific case, as I already said, I don't think this feature is actually the best solution to our problem. What we have is really a unit test, we're just lacking a way to specify that this unit test must be executed on the main thread of a program. I guess I should go raise that as a feature request for libtest.

@weihanglo
Copy link
Member

A solution could be as simple as a Cargo.toml key that makes cargo test behave as if you'd typed cargo test --features=foo,bar. As far as I can see, it doesn't need to affect how feature unification works, or any other Cargo internals.

Are you proposing something like #4942? It doesn't seem too hard in terms of supporting that. However but I've outlined some unresolved questions around that needing to be addressed.

If we look into RFC 3374 a bit deeper, there is a paragraph calling out some difficulties and open questions on the implementation side. I know it looks not too hard but things are under the surface 😞.

Anyway, thank you for bringing more info on the table.

robin-aws added a commit to smithy-lang/smithy-dafny that referenced this issue Jul 12, 2024
*Issue #, if available:*
Resolves #458

*Description of changes:*

Supports compiling the common Dafny tests in a test model for Rust. This involved having to change some properties of how we lay out the Rust crates:

* ~Removed the `dafny_impl` sub-crate and moved `implementation_from_dafny.rs` into the main `src` directory - hence replacing `::simple_boolean_dafny` with `crate::implementation_from_dafny` everywhere. I originally had this separated to better divide Dafny-generated code from Smithy-generated code, but it made implementing externs hard/impossible, and Dafny tests make use of "wrapped services" which are essentially testing-only extern shims.~
  * This turned out to be unnecessary, because it is reasonable to patch in additional `use ::simple_boolean_dafny::*;` and 
`use simple_boolean::*;` imports into the compiled tests. This is equivalent to the `pub use dafny_standard_library::implementation_from_dafny::*;` import the Makefile is adding, and will be replaced by a Dafny feature named something like `--rust-module-name` as some other other supported languages already have.
* Added ~`tests/tests_from_dafny/_wrapped.rs`~ `wrapped::client::Client` with the implementation of the "wrapped service", which is implementing the Dafny-client interface using the Rust idiomatic client.
  * Since this is only used for testing, but implements a Dafny extern that is only defined by Dafny test code, I guarded this with a `wrapped-client` feature which the Dafny-compiled integration test enables, as per rust-lang/cargo#2911 (comment)
* ~Removed `async` from the client interfaces - we'd originally kept these for better forwards-compatibility, but AFAICT it's impossible to implement the synchronous Dafny-generated trait methods with async methods. This just means that in the future we'll eventually have to provide separate async clients, but that's happened with the AWS SDKs frequently as well.~
  * Worked around this by instantiating a `current_thread` Tokio `Runtime` as per https://tokio.rs/tokio/topics/bridging instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features Area: features — conditional compilation S-propose-close Status: A team member has nominated this for closing, pending further input from the team S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests