Skip to content

Introducing limit of times a mock is available#33

Merged
declark1 merged 4 commits intoIBM:mainfrom
ferranjr:timesMatcher
Mar 26, 2025
Merged

Introducing limit of times a mock is available#33
declark1 merged 4 commits intoIBM:mainfrom
ferranjr:timesMatcher

Conversation

@ferranjr
Copy link
Copy Markdown
Contributor

As proposed in #32 this introduces a little matcher helper to limit the amount of times a matcher successfully matches.

This enables testing of retries mechanisms on a client and hopefully help other cases too.

let mock = Mock::new(|when, then| {
    when.times(3, any());
    then.ok();
})

@ferranjr ferranjr force-pushed the timesMatcher branch 2 times, most recently from f59623d to 3e196f0 Compare March 25, 2025 22:20
@declark1
Copy link
Copy Markdown
Contributor

declark1 commented Mar 25, 2025

Thanks for the contribution @ferranjr. I see the new tonic release has some breaking changes causing this build to fail (I have the version loosely set to "0", not ideal for this reason). I'll fix that on main first.

@ferranjr
Copy link
Copy Markdown
Contributor Author

@declark1 PR is now rebased with your changes

@declark1
Copy link
Copy Markdown
Contributor

declark1 commented Mar 26, 2025

I was thinking about this further and I realized that it would probably be better to implement this specific feature at the mock-level (as an option, similar to priority), rather than at the matcher-level, so that the mock has a limit rather than one of it's specific matchers.

This wasn't initially possible as we didn't have a counter implemented yet to track the number of matches. I'm merging a batch of changes now, including adding match_count to Mock.

For this, I was thinking we can:

  • Add limit: Option<usize> to Mock
    • Add with_limit() and limit() methods
  • Add limit parameter to MockSet::mock_with_options() and MockServer::mock_with_options() methods
    • Previously named mock_with_priority, but I renamed it to be more flexible if we add additional options such as this
  • Update Mock::matches() to add logic to return false if there is a limit and match_count exceeds it

The usage would then look like this:

// With Mock::new()
let mock = Mock::new(|when, then| {
    when.post().path(..).json(..);
    then.json(..);
})
.with_limit(2);

// With shorthand MockSet::mock_with_options()
let mut mocks = MockSet::new();
mocks.mock_with_options(priority, limit, |when, then|) {
    when.post().path(..).json(..);
    then.json(..);
    limit = Some(2);
}

What are your thoughts on this?

@ferranjr
Copy link
Copy Markdown
Contributor Author

Yes, makes sense to move it to mock level... I noticed the changes on your other PR, and already was thinking how this could be change.

Definitely like the approach:

// With Mock::new()
let mock = Mock::new(|when, then| {
    when.post().path(..).json(..);
    then.json(..);
})
.with_limit(2);

Although, probably from working using other mocking libraries before, I am kind of bias towards times

// With Mock::new()
let mock = Mock::new(|when, then| {
    when.post().path(..).json(..);
    then.json(..);
}).times(2);

Maybe limit gives me an idea of no more calls are allowed... where times, if follow by another mock, gives an idea of the amount of times it behaves that way?

In any case, move it Mock level makes more sense, and we can implement it with the counter you have in that PR

@ferranjr
Copy link
Copy Markdown
Contributor Author

BTW, happy to work on it later, I see you merged the changes already

@declark1
Copy link
Copy Markdown
Contributor

Thanks @ferranjr, sure it would be great if you can add this. Apologies for the re-work. I suppose times works too, I don't really have a strong opinion on it.

@ferranjr ferranjr force-pushed the timesMatcher branch 2 times, most recently from d493d3c to 32717c2 Compare March 26, 2025 22:24
Signed-off-by: Ferran Puig-Calvache <ferran.puig@coralogix.com>
@ferranjr ferranjr changed the title Introducing times matcher to enable mocker server basic state changes Introducing limit of times a mock is available Mar 26, 2025
@ferranjr
Copy link
Copy Markdown
Contributor Author

@declark1 done the rework, and while doing it the with_limit made more sense, it makes it clear you are setting the value.

Signed-off-by: Ferran Puig-Calvache <ferran.puig@coralogix.com>
@declark1 declark1 self-requested a review March 26, 2025 22:49
Copy link
Copy Markdown
Contributor

@declark1 declark1 left a comment

Choose a reason for hiding this comment

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

A couple nits to update names to limit, otherwise looks good, thanks!

Comment thread mocktail/src/mock.rs Outdated
Comment on lines +62 to +63
pub fn with_limit(mut self, times: usize) -> Self {
self.limit = Some(times);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can you update times -> limit here and below in matches()

Comment thread mocktail/src/mock.rs Outdated
}

#[test]
fn test_times() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: rename to test_limit()

Comment thread mocktail/src/mock.rs Outdated
pub priority: u8,
/// Match counter.
pub match_count: AtomicUsize,
/// Limit the times the mock will work.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: reword docstring consistent with above:
/// Limit on how many times this mock can be matched.

Signed-off-by: Ferran Puig-Calvache <ferran.puig@coralogix.com>
@ferranjr
Copy link
Copy Markdown
Contributor Author

sorted, clearly I was debating times vs limit till last second :)

Comment thread mocktail/src/mock.rs Outdated
Signed-off-by: Ferran Puig-Calvache <ferran.puig@coralogix.com>
@declark1 declark1 self-requested a review March 26, 2025 23:05
Copy link
Copy Markdown
Contributor

@declark1 declark1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@declark1 declark1 merged commit 7240725 into IBM:main Mar 26, 2025
2 checks passed
@ferranjr ferranjr deleted the timesMatcher branch March 26, 2025 23:07
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.

2 participants