search

tokio-rs / tracing

Application level tracing for Rust.
https://tracing.rs
MIT License
5.34k stars 699 forks source link

Multiple test failures in tracing-futures 0.2.3 #632

Open kentfredric opened 4 years ago

kentfredric commented 4 years ago

Hi, I'm working on a Linux Vendorization project working with rust crates, and as part of the QA of the vendorization process, we run tests (and the more tests we can run, the better).

However, I've been hitting a few failure that's non-trivial to patch out downstream, and can only be seen once other failures have been patched, so I've opted to put this all in one bug for now.

Some of my test scenarios may seem weird to you, so I don't want to pester if they're intrinsically unsupported situations, (though documentation of this could be a fix, or compile-time assertion-failures when the combination is used).

cargo test fails due to no support/mod.rs ``` Compiling tracing-futures v0.2.3 (/home/kent/.cpanm/work/1583993665.25298/tracing-futures-0.2.3) error: couldn't read src/../../tracing/tests/support/mod.rs: No such file or directory (os error 2) --> src/lib.rs:485:16 | 485 | pub(crate) mod support; | ^^^^^^^ error: aborting due to previous error error: could not compile `tracing-futures`. To learn more, run the command again with --verbose. ```

Applying the most obvious workaround for that with this hackish patch:

Disable broken tests without --cfg broken_tests ```diff diff --git a/src/lib.rs b/src/lib.rs index 4b4cc4b..d8994ba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -442,19 +442,20 @@ impl WithDispatch { } } -#[cfg(test)] +#[cfg(all(broken_tests,test))] pub(crate) use self::support as test_support; // This has to have the same name as the module in `tracing`. #[path = "../../tracing/tests/support/mod.rs"] -#[cfg(test)] +#[cfg(all(broken_tests,test))] #[allow(unreachable_pub)] pub(crate) mod support; #[cfg(test)] mod tests { + #[cfg(broken_tests)] use super::{test_support::*, *}; - #[cfg(feature = "futures-01")] + #[cfg(all(broken_tests,feature = "futures-01"))] mod futures_01_tests { use futures_01::{future, stream, task, Async, Future, Stream}; use tracing::subscriber::with_default; @@ -602,6 +603,7 @@ mod tests { use super::*; + #[cfg(broken_tests)] #[test] fn stream_enter_exit_is_reasonable() { let (subscriber, handle) = subscriber::mock() @@ -625,6 +627,7 @@ mod tests { handle.assert_finished(); } + #[cfg(broken_tests)] #[test] fn sink_enter_exit_is_reasonable() { let (subscriber, handle) = subscriber::mock() ```

Is enough to make a rudimentary test pass.

But a residual problem occurs with:

cargo test --all-features ``` Compiling tracing-futures v0.2.3 (/home/kent/.cpanm/work/1583993665.25298/tracing-futures-0.2.3) error[E0407]: method `execute` is not a member of trait `Executor` --> src/executor/futures_01.rs:46:9 | 46 | / fn execute(&self, future: F) -> Result<(), ExecuteError> { 47 | | let future = self.with_dispatch(future); 48 | | deinstrument_err!(self.inner.execute(future)) 49 | | } | |_________^ not a member of trait `Executor` error[E0407]: method `execute` is not a member of trait `Executor` --> src/executor/futures_01.rs:46:9 | 46 | / fn execute(&self, future: F) -> Result<(), ExecuteError> { 47 | | let future = self.with_dispatch(future); 48 | | deinstrument_err!(self.inner.execute(future)) 49 | | } | |_________^ not a member of trait `Executor` error[E0433]: failed to resolve: use of undeclared type or module `ExecuteError` --> src/executor/futures_01.rs:12:13 | 12 | ExecuteError::new(kind, future) | ^^^^^^^^^^^^ use of undeclared type or module `ExecuteError` ... 48 | deinstrument_err!(self.inner.execute(future)) | --------------------------------------------- in this macro invocation | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0433]: failed to resolve: use of undeclared type or module `ExecuteError` --> src/executor/futures_01.rs:12:13 | 12 | ExecuteError::new(kind, future) | ^^^^^^^^^^^^ use of undeclared type or module `ExecuteError` ... 48 | deinstrument_err!(self.inner.execute(future)) | --------------------------------------------- in this macro invocation | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0412]: cannot find type `ExecuteError` in this scope --> src/executor/futures_01.rs:46:52 | 46 | fn execute(&self, future: F) -> Result<(), ExecuteError> { | ^^^^^^^^^^^^ | ::: /home/kent/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-executor-0.1.10/src/executor.rs:67:1 | 67 | pub trait Executor { | ------------------ similarly named trait `Executor` defined here | help: a trait with a similar name exists | 46 | fn execute(&self, future: F) -> Result<(), Executor> { | ^^^^^^^^ help: possible candidates are found in other modules, you can import them into scope | 34 | use futures_01::future::ExecuteError; | 34 | use tokio::prelude::future::ExecuteError; | help: you might be missing a type parameter | 41 | impl Executor for WithDispatch | ^^^^^^^^^^^^^^ error[E0412]: cannot find type `ExecuteError` in this scope --> src/executor/futures_01.rs:46:52 | 46 | fn execute(&self, future: F) -> Result<(), ExecuteError> { | ^^^^^^^^^^^^ | ::: /home/kent/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-executor-0.1.10/src/executor.rs:67:1 | 67 | pub trait Executor { | ------------------ similarly named trait `Executor` defined here | help: a trait with a similar name exists | 46 | fn execute(&self, future: F) -> Result<(), Executor> { | ^^^^^^^^ help: possible candidates are found in other modules, you can import them into scope | 34 | use futures_01::future::ExecuteError; | 34 | use tokio::prelude::future::ExecuteError; | help: you might be missing a type parameter | 41 | impl Executor for WithDispatch | ^^^^^^^^^^^^^^ warning: unused imports: `FutureExt`, `SinkExt`, `StreamExt`, `future`, `sink`, `stream` --> src/lib.rs:635:23 | 635 | use futures::{future, sink, stream, FutureExt, SinkExt, StreamExt}; | ^^^^^^ ^^^^ ^^^^^^ ^^^^^^^^^ ^^^^^^^ ^^^^^^^^^ | note: the lint level is defined here --> src/lib.rs:74:5 | 74 | unused, | ^^^^^^ = note: `#[warn(unused_imports)]` implied by `#[warn(unused)]` warning: unused import: `tracing::subscriber::with_default` --> src/lib.rs:636:13 | 636 | use tracing::subscriber::with_default; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: unused import: `super::*` --> src/lib.rs:638:13 | 638 | use super::*; | ^^^^^^^^ error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:41:25 | 41 | impl Executor for WithDispatch | ^ unexpected type argument error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:43:21 | 43 | T: Executor>, | ^^^^^^^^^^^^^^^ unexpected type argument error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:41:25 | 41 | impl Executor for WithDispatch | ^ unexpected type argument error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:43:21 | 43 | T: Executor>, | ^^^^^^^^^^^^^^^ unexpected type argument error: aborting due to 5 previous errors Some errors have detailed explanations: E0107, E0407, E0412, E0433. For more information about an error, try `rustc --explain E0107`. error: aborting due to 5 previous errors Some errors have detailed explanations: E0107, E0407, E0412, E0433. For more information about an error, try `rustc --explain E0107`. error: could not compile `tracing-futures`. To learn more, run the command again with --verbose. warning: build failed, waiting for other jobs to finish... error: could not compile `tracing-futures`. To learn more, run the command again with --verbose. ```

Which to me, suggests some combination of features is unsupported, and warrants a compile time assertion to refuse this ( as its possible to accidentally have 2 different dependencies with 2 different 'features=[ ]' specifications, which cargo will unify and give spooky action at a distance ).

It appears the broken combination is:

cargo test --no-default-features --features "futures-01 tokio" ``` Compiling tracing-futures v0.2.3 (/home/kent/.cpanm/work/1583993665.25298/tracing-futures-0.2.3) error[E0432]: unresolved import `crate::WithDispatch` --> src/executor/futures_01.rs:34:43 | 34 | use crate::{Instrument, Instrumented, WithDispatch}; | ^^^^^^^^^^^^ | | | no `WithDispatch` in the root | help: a similar name exists in the module: `Dispatch` error[E0407]: method `execute` is not a member of trait `Executor` --> src/executor/futures_01.rs:46:9 | 46 | / fn execute(&self, future: F) -> Result<(), ExecuteError> { 47 | | let future = self.with_dispatch(future); 48 | | deinstrument_err!(self.inner.execute(future)) 49 | | } | |_________^ not a member of trait `Executor` error[E0432]: unresolved import `crate::WithDispatch` --> src/executor/futures_01.rs:34:43 | 34 | use crate::{Instrument, Instrumented, WithDispatch}; | ^^^^^^^^^^^^ | | | no `WithDispatch` in the root | help: a similar name exists in the module: `Dispatch` error[E0407]: method `execute` is not a member of trait `Executor` --> src/executor/futures_01.rs:46:9 | 46 | / fn execute(&self, future: F) -> Result<(), ExecuteError> { 47 | | let future = self.with_dispatch(future); 48 | | deinstrument_err!(self.inner.execute(future)) 49 | | } | |_________^ not a member of trait `Executor` error[E0433]: failed to resolve: use of undeclared type or module `ExecuteError` --> src/executor/futures_01.rs:12:13 | 12 | ExecuteError::new(kind, future) | ^^^^^^^^^^^^ use of undeclared type or module `ExecuteError` ... 48 | deinstrument_err!(self.inner.execute(future)) | --------------------------------------------- in this macro invocation | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0433]: failed to resolve: use of undeclared type or module `Box` --> src/executor/futures_01.rs:61:26 | 61 | let future = Box::new(future.instrument(self.span.clone())); | ^^^ use of undeclared type or module `Box` error[E0433]: failed to resolve: use of undeclared type or module `ExecuteError` --> src/executor/futures_01.rs:12:13 | 12 | ExecuteError::new(kind, future) | ^^^^^^^^^^^^ use of undeclared type or module `ExecuteError` ... 48 | deinstrument_err!(self.inner.execute(future)) | --------------------------------------------- in this macro invocation | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error[E0433]: failed to resolve: use of undeclared type or module `Box` --> src/executor/futures_01.rs:205:26 | 205 | let future = Box::new(self.with_dispatch(future)); | ^^^ use of undeclared type or module `Box` error[E0433]: failed to resolve: use of undeclared type or module `Box` --> src/executor/futures_01.rs:61:26 | 61 | let future = Box::new(future.instrument(self.span.clone())); | ^^^ use of undeclared type or module `Box` error[E0412]: cannot find type `ExecuteError` in this scope --> src/executor/futures_01.rs:46:52 | 46 | fn execute(&self, future: F) -> Result<(), ExecuteError> { | ^^^^^^^^^^^^ | ::: /home/kent/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-executor-0.1.10/src/executor.rs:67:1 | 67 | pub trait Executor { | ------------------ similarly named trait `Executor` defined here | help: a trait with a similar name exists | 46 | fn execute(&self, future: F) -> Result<(), Executor> { | ^^^^^^^^ help: possible candidates are found in other modules, you can import them into scope | 34 | use futures_01::future::ExecuteError; | 34 | use tokio::prelude::future::ExecuteError; | help: you might be missing a type parameter | 41 | impl Executor for WithDispatch | ^^^^^^^^^^^^^^ error[E0412]: cannot find type `Box` in this scope --> src/executor/futures_01.rs:58:21 | 58 | future: Box + 'static + Send>, | ^^^ not found in this scope error[E0412]: cannot find type `Box` in this scope --> src/executor/futures_01.rs:202:21 | 202 | future: Box + 'static + Send>, | ^^^ not found in this scope error[E0433]: failed to resolve: use of undeclared type or module `Box` --> src/executor/futures_01.rs:205:26 | 205 | let future = Box::new(self.with_dispatch(future)); | ^^^ use of undeclared type or module `Box` warning: unused import: `self::no_std::*` --> src/stdlib.rs:14:16 | 14 | pub(crate) use self::no_std::*; | ^^^^^^^^^^^^^^^ | note: the lint level is defined here --> src/lib.rs:74:5 | 74 | unused, | ^^^^^^ = note: `#[warn(unused_imports)]` implied by `#[warn(unused)]` warning: unused import: `tracing::dispatcher` --> src/lib.rs:90:5 | 90 | use tracing::dispatcher; | ^^^^^^^^^^^^^^^^^^^ warning: unused import: `Dispatch` --> src/lib.rs:91:15 | 91 | use tracing::{Dispatch, Span}; | ^^^^^^^^ error[E0412]: cannot find type `ExecuteError` in this scope --> src/executor/futures_01.rs:46:52 | 46 | fn execute(&self, future: F) -> Result<(), ExecuteError> { | ^^^^^^^^^^^^ | ::: /home/kent/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-executor-0.1.10/src/executor.rs:67:1 | 67 | pub trait Executor { | ------------------ similarly named trait `Executor` defined here | help: a trait with a similar name exists | 46 | fn execute(&self, future: F) -> Result<(), Executor> { | ^^^^^^^^ help: possible candidates are found in other modules, you can import them into scope | 34 | use futures_01::future::ExecuteError; | 34 | use tokio::prelude::future::ExecuteError; | help: you might be missing a type parameter | 41 | impl Executor for WithDispatch | ^^^^^^^^^^^^^^ error[E0412]: cannot find type `Box` in this scope --> src/executor/futures_01.rs:58:21 | 58 | future: Box + 'static + Send>, | ^^^ not found in this scope error[E0412]: cannot find type `Box` in this scope --> src/executor/futures_01.rs:202:21 | 202 | future: Box + 'static + Send>, | ^^^ not found in this scope warning: unused import: `self::no_std::*` --> src/stdlib.rs:14:16 | 14 | pub(crate) use self::no_std::*; | ^^^^^^^^^^^^^^^ | note: the lint level is defined here --> src/lib.rs:74:5 | 74 | unused, | ^^^^^^ = note: `#[warn(unused_imports)]` implied by `#[warn(unused)]` warning: unused import: `tracing::dispatcher` --> src/lib.rs:90:5 | 90 | use tracing::dispatcher; | ^^^^^^^^^^^^^^^^^^^ warning: unused import: `Dispatch` --> src/lib.rs:91:15 | 91 | use tracing::{Dispatch, Span}; | ^^^^^^^^ error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:41:25 | 41 | impl Executor for WithDispatch | ^ unexpected type argument error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:43:21 | 43 | T: Executor>, | ^^^^^^^^^^^^^^^ unexpected type argument error: aborting due to 10 previous errors Some errors have detailed explanations: E0107, E0407, E0412, E0432, E0433. For more information about an error, try `rustc --explain E0107`. error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:41:25 | 41 | impl Executor for WithDispatch | ^ unexpected type argument error[E0107]: wrong number of type arguments: expected 0, found 1 --> src/executor/futures_01.rs:43:21 | 43 | T: Executor>, | ^^^^^^^^^^^^^^^ unexpected type argument error: aborting due to 10 previous errors Some errors have detailed explanations: E0107, E0407, E0412, E0432, E0433. For more information about an error, try `rustc --explain E0107`. error: could not compile `tracing-futures`. To learn more, run the command again with --verbose. warning: build failed, waiting for other jobs to finish... error: could not compile `tracing-futures`. To learn more, run the command again with --verbose. ```
hawkw commented 4 years ago

Thanks for the reports, this is very helpful --- I'd definitely like all our tests to work even in edge cases, so we'll try to fix these!

cargo test fails due to no support/mod.rs

I'm guessing that your vendorization process involves building crates outside of the upstream repo? This failure is due to using a #[path="..."] attribute to include shared test-support code: f you are building out-of-tree, the test support code will not be present at the expected location.

Some options for fixing this:

But a residual problem occurs with: cargo test --all-features

Which to me, suggests some combination of features is unsupported, and warrants a compile time assertion to refuse this ( as its possible to accidentally have 2 different dependencies with 2 different 'features=[ ]' specifications, which cargo will unify and give spooky action at a distance ).

It appears the broken combination is: cargo test --no-default-features --features "futures-01 tokio"

It looks like the issue here is that the --no-default-features is disabling the std feature as well as futures 0.3 support, which adds a #![no_std] flag and therefore breaks compilation with futures 0.1, which requires std. I think we should be able to fix this by changing the futures 0.1 and tokio features to require the std feature flag. We have a CI step that tests the full powerset of feature combinations for this crate, so I'm honestly pretty surprised this wasn't caught on CI...I wonder what's different between your environment and the CI test.

hawkw commented 4 years ago

We have a CI step that tests the full powerset of feature combinations for this crate, so I'm honestly pretty surprised this wasn't caught on CI...I wonder what's different between your environment and the CI test.

Oh, I see what's the matter! CI only runs cargo check against the feature powerset: https://github.com/tokio-rs/tracing/blob/12fde2dd52308b3abf363256444aedfa74433171/.github/workflows/CI.yml#L86

This means that we only check if the main crate code compiles with those feature combinations. All these issues are only when compiling tests, and CI isn't currently checking that tests compile across feature combinations. We should fix this.

kentfredric commented 4 years ago

I'm guessing that your vendorization process involves building crates outside of the upstream repo? This failure is due to using a #[path="..."] attribute to include shared test-support code: f you are building out-of-tree, the test support code will not be present at the expected location.

Indeed. We prefer to use "published sources" as they're predictable and immutable, and more mirror friendly, and given the "published sources" are what everyone actually uses, it makes more sense to test the published content works as expected ( which can in some cases vary from upstreams repo at the time of publish ).

Just unfortunately, rust ecosystem norms don't tend to prioritize the ability to test this scenario (which weakens the ability to verify the published sources are reliable).

So anything that improves this norm is welcome :)

hawkw commented 4 years ago

633 should fix the issue with feature flags. We'll try to find a better solution for sharing the test-support code, but that may take a little time, especially since it's going to take some effort to clean that up so it can be a published API rather than internal. Thanks again for bringing this up!

davidbarsky commented 4 years ago

633 should fix the issue with feature flags. We'll try to find a better solution for sharing the test-support code, but that may take a little time, especially since it's going to take some effort to clean that up so it can be a published API rather than internal. Thanks again for bringing this up!

Given https://github.com/tokio-rs/tracing/pull/633#discussion_r392459073, should (could?) we publish an unstable, use-at-your own risk version of tracing-test? If we need to make a breaking change to tracing-test, we can do a minor version bump of crates that depend on it. Its useful as-is, even if its not the most polished code.