docs: update docs with synchronization example #337

TomPridham · 2021-09-20T22:40:55Z

just adds a link to the synchronization example in the main lib.rs doc

asomers · 2021-09-20T23:02:28Z

This is probably a good addition, once you fix rustdoc. Even better though would be dedicated synchronization examples, perhaps in the examples directory.
Also, I've considered building synchronization into Mockall. Basically, creating a Context object would automatically acquire a lock. However, these would be vulnerable to lock-order reversals and other bugs, since the user would probably be unaware of them. What do you think of that idea?

mockall/src/lib.rs

TomPridham · 2021-10-12T19:41:44Z

@asomers, i added an example to the examples folder. let me know what you think.

nightly failed on some stuff in mockall_derive that i didn't touch. not sure what you want to do about that

personally i would like some sort of automatic locking. it's noisy to have to have to create a lock and then acquire it in every test. it's also vulnerable to new people joining the codebase. if they don't copy an existing test mod, read the readme or find the line in the docs that talks about synchronization they might end up with some "works on my machine" problems

my opinion would probably change if the tests deadlocked a lot. but i wonder if it would be possible to create an attribute or macro that was like #[mockall_synchronization] that basically just did the work of creating the mutex and adding the lock acquisition to the start of every test in the mod with the attribute

mockall/examples/synchronization.rs

mockall/src/lib.rs

asomers · 2021-10-12T21:19:25Z

mockall/examples/synchronization.rs

+    fn get_lock(m: &'static Mutex<()>) -> MutexGuard<'static, ()> {
+        match m.lock() {
+            Ok(guard) => guard,
+            Err(poisoned) => poisoned.into_inner(),


Nice touch.

asomers · 2021-10-12T21:23:28Z

Looks like nightly clippy got pickier. I'll pacify it in a separate PR.

asomers · 2021-10-12T22:36:41Z

Ok, if you rebase on master now, it should fix your compile problems.

TomPridham · 2021-10-14T21:10:53Z

ok, @asomers should be good now

TomPridham · 2021-10-14T22:53:43Z

does the synchronization need to happen between all different test mods or are the different test modules isolated? i have two files with test modules a.rs and b.rs that both mock c.rs. does the synchronization need to make sure that only one test is running at a time between a AND b or is it enough to make sure that only one test at a time is running per module?

TomPridham · 2021-10-14T23:07:33Z

it seems like the synchronization needs to happen between all modules that are using the same mock. i'll update the example tomorrow to better illustrate that. i just did some refactoring that exposed that potential failure mode in our own codebase 😅

TomPridham · 2021-10-14T23:14:49Z

i'm also unsure if my get_lock function is working as intended, i still seem to get poison errors sometimes from the automock attribute. unsure if the backtrace is just confused or if there is another mutex in being used in the automock that is getting poisoned as well

asomers · 2021-10-15T01:20:18Z

does the synchronization need to happen between all different test mods or are the different test modules isolated? i have two files with test modules a.rs and b.rs that both mock c.rs. does the synchronization need to make sure that only one test is running at a time between a AND b or is it enough to make sure that only one test at a time is running per module?

It depends on how the two modules are linked. If they're in the same process, then you need synchronization between them. If they are separate processes, then you don't. By default, Cargo will create a separate program for every file in the tests directory, and all tests within that program get executed as part of the same process.

mockall/examples/synchronization.rs

asomers · 2021-10-15T01:23:33Z

Oh, and for unit tests Cargo by default creates one program for a crate's library's unit tests, and one program for each of the crate's binary's unit tests.

asomers · 2022-01-22T16:05:44Z

mockall/src/lib.rs

@@ -836,7 +836,8 @@
 //!
 //! Mockall can also mock static methods.  But be careful!  The expectations are
 //! global.  If you want to use a static method in multiple tests, you must
-//! provide your own synchronization.  For ordinary methods, expectations are
+//! provide your own synchronization. See the [`synchronization example`](examples/synchronization.rs)


This link will be dead, because rustdoc doesn't include files in the examples/ directory. Instead, it's probably best to link to the file on Github.

i just linked to where the file would be once this is merged in. should i link to a specific commit hash or is that fine?

I think it's best not to provide a specific hash. We might update the example later, and it would be easy to forget to update the link.

okay, updated to use my previous commit as the reference

It looks like you've pinned a specific commit. Don't you think it's better to point to the latest version?

oh, oops. i misread your comment. i thought you wanted me to include a specific commit hash. i can revert that to point to the latest version

@asomers does this seem good to go now?

asomers

Yep, it looks good now.

TomPridham requested a review from asomers as a code owner September 20, 2021 22:40

TomPridham force-pushed the patch-1 branch 2 times, most recently from 48d3914 to aaf4cc8 Compare October 12, 2021 19:14

TomPridham commented Oct 12, 2021

View reviewed changes

mockall/src/lib.rs Outdated Show resolved Hide resolved

asomers requested changes Oct 12, 2021

View reviewed changes

TomPridham force-pushed the patch-1 branch 2 times, most recently from e0696b0 to 8298c23 Compare October 14, 2021 21:01

asomers requested changes Oct 15, 2021

View reviewed changes

mockall/examples/synchronization.rs Outdated Show resolved Hide resolved

docs: add synchronization example

7955b5f

TomPridham force-pushed the patch-1 branch from 8298c23 to 7955b5f Compare January 20, 2022 19:52

asomers requested changes Jan 22, 2022

View reviewed changes

TomPridham force-pushed the patch-1 branch 2 times, most recently from 6e01e72 to 0999b53 Compare January 26, 2022 00:08

docs: directly link example in github

48e4229

TomPridham force-pushed the patch-1 branch from 81b741a to 48e4229 Compare January 27, 2022 17:04

asomers approved these changes Feb 4, 2022

View reviewed changes

asomers merged commit 0fb6367 into asomers:master Feb 4, 2022

TomPridham deleted the patch-1 branch February 15, 2022 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: update docs with synchronization example #337

docs: update docs with synchronization example #337

TomPridham commented Sep 20, 2021

asomers commented Sep 20, 2021

TomPridham commented Oct 12, 2021

asomers Oct 12, 2021

asomers commented Oct 12, 2021

asomers commented Oct 12, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

asomers commented Oct 15, 2021

asomers commented Oct 15, 2021

asomers Jan 22, 2022

TomPridham Jan 25, 2022

asomers Jan 25, 2022

TomPridham Jan 26, 2022

asomers Jan 26, 2022

TomPridham Jan 27, 2022

TomPridham Feb 2, 2022

asomers left a comment

docs: update docs with synchronization example #337

docs: update docs with synchronization example #337

Conversation

TomPridham commented Sep 20, 2021

asomers commented Sep 20, 2021

TomPridham commented Oct 12, 2021

Choose a reason for hiding this comment

asomers commented Oct 12, 2021

asomers commented Oct 12, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

TomPridham commented Oct 14, 2021

asomers commented Oct 15, 2021

asomers commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers left a comment

Choose a reason for hiding this comment