Make test output appear in the correct failing test #2901
Conversation
|
I think |
|
Yes it is: #2904 |
|
Haven't reviewed completely yet (is it ready for review?), but I think it would be nice to have an let (log_tx, _log_sink) = LogSink::create();
let alice = TestContext::builder()
.configure_alice()
.with_log_sink(log_tx.clone())
.build()
.await;
let bob = TestContext::builder()
.configure_bob()
.with_log_sink(log_tx)
.build()
.await;we can write: let acfactory = AcFactory::new().await;
let alice = acfactory.new_alice().await;
let bob = acfactory.new_bob().await;or maybe even: let acfactory = AcFactory::new().await;
let (alice, bob) = acfactory.new_alice_and_bob().await;(similar to The last version has the slight disadvantage that people could think that it's possible to write One other, slightly less elegant but most ergonomic option would be to allow: let (alice, bob) = TestContext::new_alice_and_bob().await;and just putting the |
Definitely ready for review, otherwise I'd have left it in draft.
As you can tell I like being explicit. It does have its benefits to push
This could be nice, sure. Though I'm worried it's too easy to start doing
I'm not a particular fan of tuple-returns. The only case where they are common are in constructors where you must give two things back, e.g. Here it is simply returning twice the same to save some typing. I don't think this is a great tradeoff.
This is breaking a lot of boundaries, Still, trying to make construction shorter and avoid misusing it maybe could look something like this: let manager = AcManager::new().await;
let alice: &TestContext = manager.ac_alice().await;
let also_alice: Option<&TestContext> = manager.get("alice");
let charlie = manager.ac("[email protected]").await;Here you would only ever get a reference to a One problem this does have again is that constructors on it start sprawling again, imagine if we need to add a way to create a new account but also give it a specifically generated autocrypt key. We'd again end up with something like |
|
+1 for your version with AcManager 👍! If we want to, we can then also do things like, when a test fails print some extra info about the contact (like, the list of chats as the REPL would show them) |
Can we treat the AcManager as purely additive to the stuff here and do it in a separate PR? |
src/test_utils.rs
Outdated
| /// Reference to implicit [`LogSink`] so it is dropped together with the context. | ||
| /// | ||
| /// Only used if no explicit [`LogSink`] was given during construction. This is a | ||
| /// convenience in case only a single [`TestContext`] is used to avoid dealing with | ||
| /// [`LogSink`]. Never read, thus "dead code", since the only purpose is to control | ||
| /// when Drop is invoked.. | ||
| #[allow(dead_code)] | ||
| log_sink: Option<LogSink>, |
There was a problem hiding this comment.
Not sure about this approach, it has a really high cognitive complexity ("if there is an explicit LogSink, then there is not a LogSink here, but somewhere else, namely directly in the test, but we always have a Sender for the logs, which is not visible here because it goes into the event_senders. A LogSink is the thing that receives the events and prints them at the end, while the Sender is the thing with which you can send() the events when you get them from the core.").
My alternative isn't great either, but still better I think. Or maybe we find yet another alternative. Or if not some comment that explains the bigger picture could help, I could also help with writing this in this case.
My alternative:
Remove log_sink here. If there is an explicit log sink given, don't change anything.
If there is no explicit log sink given, just print the events as we did before.
We should probably make the new log sink sender an explicit Option<Sender<Event>> then instead of putting it into event_senders in order to still know whether we have an explicit log sink or not.
(my approach will also has the disadvantage that when running multiple tests the output will be as splattered as before this PR, and the advantage that when running a single test the output will be as nice as before - the splattering never was a problem for me personally as I rarely run multiple tests at once while debugging, so maybe I'm biased).
--
The sentence
Only used if no explicit [
LogSink] was given during construction
is technically wrong and confused me when reading. Technically correct (but not a helpful comment) would be
Only used if 'log_sender: Option<Sender>' was
Noneduring construction'
note that the caller doesn't provide a LogSink but a Sender<Event>.
There was a problem hiding this comment.
(my approach will also has the disadvantage that when running multiple tests the output will be as splattered as before this PR, and the advantage that when running a single test the output will be as nice as before - the splattering never was a problem for me personally as I rarely run multiple tests at once while debugging, so maybe I'm biased).
So yes, this is kind of the entire crux of this PR as you point out here. Without an implicit LogSink the output will be splattered and highly confusing (to me at least, made worse by the fact that if you have test_spam and test_spam_and_ham it seems impossible to only run test_spam so when both are failing your test output is useless).
The point here is that by creating the LogSink implicitly the vast majority of the tests do not have to care or be changed at all to start behaving correctly. And also all tests will have their logs contained in their own test avoiding leaking to other tests. We only need to change tests which use multiple TestContext's and where we want the output interleaved.
The sentence
Only used if no explicit [
LogSink] was given during constructionis technically wrong and confused me when reading. Technically correct (but not a helpful comment) would be
Only used if 'log_sender: Option' was
Noneduring construction'note that the caller doesn't provide a
LogSinkbut aSender<Event>.
So I tried to improve the comments, hopefully this makes it less confusing and also reduces the cognitive complexity. I really would like to avoid degrading the behaviour here because it is explicitly the main goal of this PR.
There was a problem hiding this comment.
if you have
test_spamandtest_spam_and_hamit seems impossible to only runtest_spam
You can use e.g. cargo test -- dc_tools::tests::test_parse_receive_headers --exact to run test_parse_receive_headers but not test_parse_receive_headers_integration. However, of course that's quite a lot to type just to run one test and I'm personally usually using the "Run" button which rust-analyzer adds to VSCode instead in this case because I don't want to type all of this.
The comments are a lot better now 👍
There was a problem hiding this comment.
TIL about --exact, thanks (i suspected this must be possible somehow)
There was a problem hiding this comment.
ah, i'm not yet used to rust-analyser's adding of those lens things. they also work in emacs and indeed use --exact
I'm not going to remove the |
Without this the test output is written somewhere random and ends up in the wrong test report. This buffers all the log output and prints it inside the test on dropping the LogSink, or on dropping the TestContext if no explicit LogSink was created.
There are too many ways to create a TestContext, this introduces a TestContextBuilder to try and keep this shorter. It also cleans up the existing constructors keeping only the commonly used ones.
This replaces the EventSink callbacks with simple channel senders. This simplifies the TestContext a lot as that is much simpler to handle. It then also removes the special-casing of the LogSink since it now is another even sender, only injected at the very start.
I considered removing it from the context by default, but the migration test really wants to have the tracker initialised from the very first event and not after the context is initialised. It is easier for now to leave it hardcoded instead of adding an API to explicitly require enabling it via the builder.
Saves a bit of repitions.
See #2901 (comment) This reduces boilerplate code again therefore, improving the signal-noise-ratio and reducing the mental barrier to start writing a unit test. Slightly off-topic: I didn't add any advanced functions like `manager.get("alice");` because they're not needed yet; however, once we have the AcManager we can think about fancy things like: ```rust acm.send_text(&alice, "Hi Bob, this is Alice!", &bob); ``` which automatically lets bob receive the message. However, this may be less useful than it seems at first, since most of the tests I looked at wouldn't benefit from it, so at least I won't do it until I have a test that would benefit from it.
See #2901 (comment) This reduces boilerplate code again therefore, improving the signal-noise-ratio and reducing the mental barrier to start writing a unit test. Slightly off-topic: I didn't add any advanced functions like `manager.get("alice");` because they're not needed yet; however, once we have the AcManager we can think about fancy things like: ```rust acm.send_text(&alice, "Hi Bob, this is Alice!", &bob); ``` which automatically lets bob receive the message. However, this may be less useful than it seems at first, since most of the tests I looked at wouldn't benefit from it, so at least I won't do it until I have a test that would benefit from it.
* Add AcManager See #2901 (comment) This reduces boilerplate code again therefore, improving the signal-noise-ratio and reducing the mental barrier to start writing a unit test. Slightly off-topic: I didn't add any advanced functions like `manager.get("alice");` because they're not needed yet; however, once we have the AcManager we can think about fancy things like: ```rust acm.send_text(&alice, "Hi Bob, this is Alice!", &bob); ``` which automatically lets bob receive the message. However, this may be less useful than it seems at first, since most of the tests I looked at wouldn't benefit from it, so at least I won't do it until I have a test that would benefit from it. * Remove unnecessary RefCell * Rename AcManager to TestContextManager * Don't store TestContext's in a vec for now as we don't need this; we can re-add it later * Rename acm -> tcm
Tests made sure to log their events, however when you have multiple failures the results were all over the place. This introduces a LogSink to ensure the test failures look good and each has their own log reports. If you only have a single context nothing changes since the TestContext keeps a reference to the LogSink and since the TestContext is dropped at the end of each test this works correctly. If you have multiple TestContexts in a test and want to see the logs intermingled, both TestContexts need to use the same LogSink and you need to create it explicitly.
It is best to review this commit-by-commit, because one thing led to another: