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

Put mocking functionality into a crate #2009

Merged
merged 1 commit into from
Mar 22, 2022
Merged

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Mar 21, 2022

... instead of #[path = ""] importing it everywhere.

Make sure to use a diff review tool that understands file renaming 😅 (GitHub's diff view does.)

Motivation

Transparency: I want to use the mocking functionality in the development of a tracing component out-of-tree.

Additionally, this reduces the use of #[path] mod and file multiple-inclusion, which aren't that great of a practice.

Solution

The tracing test support module was already well self-contained, due to being #[path] mod used in multiple places. As such, extracting it to its own crate is rather mechanical, with no surprising blockers.

We additionally move the tracing-futures support module contents into tracing_mock, for convenience. The one function which relies on tokio-test is made optional.

It's a reasonable result for this functionality to stay unpublished, and only used inside the repo, but pulling it out into a directly reusable chunk instead of abusing the module system to reuse it via multiple-inclusion is an improvement to code structure and modularization.

@CAD97 CAD97 requested review from hawkw, davidbarsky and a team as code owners March 21, 2022 05:47
@CAD97 CAD97 force-pushed the tracing-mock branch 3 times, most recently from 1d0255c to 63cd43c Compare March 21, 2022 06:14
@hawkw
Copy link
Member

hawkw commented Mar 21, 2022

Can a crate be published if it has dev dependencies that are not published to crates.io?

The motivation for the previous approach was to avoid path-only crate dependencies so that the crates could be published, since it is not possible to publish a crate that depends on a crate that is not itself published. If this requirement doesn't exist for dev-dependencies, I'm happy to make this change now. Otherwise, it would require committing to publishing the test-support code, which I think might require additional work to make that API something we can commit to maintaining.

@CAD97
Copy link
Contributor Author

CAD97 commented Mar 22, 2022

Can a crate be published if it has dev dependencies that are not published to crates.io?

Actually, yes!

https://crates.io/crates/cad97-publish-test (with apologies for the empty publish, but I don't know how to test with https://staging.crates.io)

https://docs.rs/crate/cad97-publish-test/0.1.0/source/Cargo.toml.orig

[package]
name = "cad97-publish-test"
version = "0.1.0"
edition = "2021"

description = "this is a publishing test; do not use"
license = "MIT OR Apache-2.0 OR Unlicense"

[dependencies]

[dev-dependencies]
cad97-publish-test-dep = { path = "cad97-publish-test-dep" }

get cleaned up to remove the path-only dev-dep somewhere in the publish pipeline:

https://docs.rs/crate/cad97-publish-test/0.1.0/source/Cargo.toml

[dependencies]

[dev-dependencies]

[package]
description = "this is a publishing test; do not use"
edition = "2021"
license = "MIT OR Apache-2.0 OR Unlicense"
name = "cad97-publish-test"
resolver = "2"
version = "0.1.0"

(This doesn't work for workspace hack crates, though, because that wants to set the dependency edges specifically for [dependencies] and [build-dependencies] (and doing so is necessary for resolver = "2").)

... instead of #[path = ""] importing it everywhere
@CAD97
Copy link
Contributor Author

CAD97 commented Mar 22, 2022

Context update:

For tracing-filter, what I actually apparently want is a mock/testing Subscribe (stable Layer).

I still think this is an organization benefit to the tracing repository, but (as I have just found out) it is not necessary/sufficient1 for my needs anymore.

Footnotes

  1. It would probably still be useful to have an impl Collect + LookupSpan (probably just wrapping Registry) which allows you to test that what callbacks the registry is receiving is what you expect, as well as an impl Subscribe to test that layers at that point in the stack are getting the callbacks you expect, but my current needs are just for the layer portion.

@hawkw
Copy link
Member

hawkw commented Mar 22, 2022

For tracing-filter, what I actually apparently want is a mock/testing Subscribe (stable Layer).

This exists on v0.1.x: https://github.com/tokio-rs/tracing/blob/v0.1.x/tracing-subscriber/tests/support.rs

Once this PR is merged, as part of backporting it to v0.1.x, we would update the tracing-subscriber test support code to depend on the mock crate. Unfortunately, it cannot easily be moved into that crate, because this would create a circular dependency between tracing-subscriber's tests, which depend on the mock Layer, and the mock layer implementation, which depends on tracing-subscriber for the definition of the Layer trait. But, the same mock Layer code could be re-implemented using the mock crate; it's relatively simple.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Given that this won't interfere with publishing crates that depend on the test code, I'm happy to move forwards with this change! Not having to use #[path="..."] attributes everywhere definitely makes the test code look a lot nicer.

I'm definitely also open to considering publishing the tracing-mock crate for third-party tracing-related crates to depend on from crates.io, something people have expressed a desire for in the past (#539, #793). However, before we do that, I'd want to put some work into making the APIs exposed by that crate release-ready --- right now, they don't do much to ensure that they are forward-compatible, and there isn't any documentation.

We should make any actual code changes to the mocking code in a separate branch, though, so that it's not part of the diff that moves the files around.

@hawkw hawkw merged commit ced9302 into tokio-rs:master Mar 22, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Mar 23, 2022

Unfortunately, it cannot easily be moved into that crate, because this would create a circular dependency between tracing-subscriber's tests, which depend on the mock Layer, and the mock layer implementation, which depends on tracing-subscriber for the definition of the Layer trait.

Are you sure about that? The dependency chain would be

tracing-subscriber (test) -> tracing-mock -> tracing-subscriber

Which is monodirectional. In fact, testing in a local workspace, it seems to work even for #[cfg(test)]:

cad97-publish-test

[package]
name = "cad97-publish-test"

[dev-dependencies]
cad97-publish-test-dep = { path = "cad97-publish-test-dep" }
pub fn in_main_crate() {}

#[test]
fn test() {
    cad97_publish_test_dep::in_test_crate();
}

cad97-publish-test-dep

[package]
name = "cad97-publish-test-dep"

[dependencies]
cad97-publish-test = { path = ".." }
pub fn in_test_crate() {
    cad97_publish_test::in_main_crate();
}

cargo test

D:\repos\cad97\cad97-publish-test [main]> cargo test
    Finished test [unoptimized + debuginfo] target(s) in 0.01s
     Running unittests (D:\.rust\target\debug\deps\cad97_publish_test-437ba04881f66aec.exe)

running 1 test
test test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

and I was able to publish the crate with the alleged circular dependency just fine.

(I do not know how exactly this behaves with a non-path-only dependency, though; that might claim a cycle and refuse to publish.)

@CAD97 CAD97 deleted the tracing-mock branch March 23, 2022 21:11
hawkw pushed a commit that referenced this pull request Mar 29, 2022
... instead of `#[path = ""]` importing it everywhere.

Make sure to use a diff review tool that understands file renaming 😅
(GitHub's diff view does.)

## Motivation

Transparency: I want to use the mocking functionality in the development
of a tracing component out-of-tree.

Additionally, this reduces the use of `#[path] mod` and file
multiple-inclusion, which aren't that great of a practice.

## Solution

The tracing test support module was already well self-contained, due to
being `#[path] mod` used in multiple places. As such, extracting it to
its own crate is rather mechanical, with no surprising blockers.

We additionally move the tracing-futures support module contents into
tracing_mock, for convenience. The one function which relies on
tokio-test is made optional.

It's a reasonable result for this functionality to stay unpublished, and
only used inside the repo, but pulling it out into a directly reusable
chunk instead of abusing the module system to reuse it via
multiple-inclusion is an improvement to code structure and
modularization.
hawkw pushed a commit that referenced this pull request Mar 29, 2022
... instead of `#[path = ""]` importing it everywhere.

Make sure to use a diff review tool that understands file renaming 😅
(GitHub's diff view does.)

## Motivation

Transparency: I want to use the mocking functionality in the development
of a tracing component out-of-tree.

Additionally, this reduces the use of `#[path] mod` and file
multiple-inclusion, which aren't that great of a practice.

## Solution

The tracing test support module was already well self-contained, due to
being `#[path] mod` used in multiple places. As such, extracting it to
its own crate is rather mechanical, with no surprising blockers.

We additionally move the tracing-futures support module contents into
tracing_mock, for convenience. The one function which relies on
tokio-test is made optional.

It's a reasonable result for this functionality to stay unpublished, and
only used inside the repo, but pulling it out into a directly reusable
chunk instead of abusing the module system to reuse it via
multiple-inclusion is an improvement to code structure and
modularization.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
... instead of `#[path = ""]` importing it everywhere.

Make sure to use a diff review tool that understands file renaming 😅
(GitHub's diff view does.)

Transparency: I want to use the mocking functionality in the development
of a tracing component out-of-tree.

Additionally, this reduces the use of `#[path] mod` and file
multiple-inclusion, which aren't that great of a practice.

The tracing test support module was already well self-contained, due to
being `#[path] mod` used in multiple places. As such, extracting it to
its own crate is rather mechanical, with no surprising blockers.

We additionally move the tracing-futures support module contents into
tracing_mock, for convenience. The one function which relies on
tokio-test is made optional.

It's a reasonable result for this functionality to stay unpublished, and
only used inside the repo, but pulling it out into a directly reusable
chunk instead of abusing the module system to reuse it via
multiple-inclusion is an improvement to code structure and
modularization.
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