tracing(test): rename mock classes used in tests to reflect their actual purpose

[yaahc]

Feature Request

Currently much of tracing's test code uses subscriber::mock() to assert what is essentially a test plan. However this module and the methods used in setting up the test plan are called "mock", which is a term generally reserved for dummy impls of pieces of your system. A mock subscriber should act like a subscriber, not track what one is doing.

Crates

All crates that use subscriber::mock() and its related functions.

Motivation

To make test code less confusing to new comers.

Proposal

These types, modules, etc. should be renamed to better reflect that they are a set of asserts and test plans.

Alternatives

Alternatively, we could not do that.

[hawkw]

This seems good to me. I think subscriber::mock() could be changed to subscriber::test(), and the other functions (e.g. span::mock(), event::mock(), etc) to span::expect(), event::expect(), etc. However, I'm open to other naming suggestions.

We might even want to put these functions in a new expect module, so that the usage is expect::span() rather than span::expect(), like a cute little DSL. Not sure if this is worth the effort though.

It might also be nice to try and reduce some of the verbosity of this API a little, though it's not a high priority. For example, right now, if we want to expect a particular span, we say span::mock().named("my_span"). Since we almost always expect a name for the span, we might want to just change this to span::expect("my_span").

[yaahc]

I like the cute little DSL suggestion and the names suggested here. I'm only worried about the comment "we almost always expect a name for a span". I assume the default would be expect::span(name) but if there are cases when we would not want a name that needs to be part of the API somehow.

I'm thinking we could use this signature fn span<'a>(name: impl Into<Option<&'a str>>) so we can call it as expect::span("name") or expect::span(None).

[hawkw]

I'm only worried about the comment "we almost always expect a name for a span". I assume the default would be expect::span(name) but if there are cases when we would not want a name that needs to be part of the API somehow.

I think there are a few cases where we don't currently expect a specific name, but we can also just update those tests to expect the name --- we are always expecting a particular span that's created in the test, so if we expect any other attributes, we can also expect the name as well.

The only reason not to is that we might want tests to focus specifically on the qualities they're testing. I don't know if this really matters.

[hawkw]

We might even want to put these functions in a new expect module, so that the usage is expect::span() rather than span::expect(), like a cute little DSL. Not sure if this is worth the effort though.

As an alternative proposal: WDYT of getting rid of the test subscriber builder's span, enter, exit, etc functions, and replacing them with

    fn expect<I: Into<Expect>(self, I) -> Self 

or similar?

That way, we would write tests like

let subscriber = subscriber::test()
   .expect(new_span(...))
   .expect(enter(...))
   .expect(exit(...))
   ...

This might be an excessive amount of work for the potential benefit, though.

[yaahc]

That looks super pretty, so I'm all for it. I'll leave it up to you to gauge if the amount of work for this version vs the other version is worth it.

[hawkw]

@yaahc if you're interested in making the change, it's entirely up to you to decide how far you're willing to go :)

[inanna-malick]

I'd be interested in putting together a DSL using the Into<Expect> pattern suggested earlier (seems like a good way to get started with OS rust stuff). @yaahc, would you mind if I base that work on your renaming commit (e3663c9)?

[yaahc]

@pkinsky that sounds awesome, go for it!