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

Replace nap.sleep with a method to allow mocking after import #236

Merged
merged 3 commits into from
Oct 24, 2020

Conversation

asqui
Copy link
Contributor

@asqui asqui commented Jun 7, 2020

Fixes #228

@asqui asqui force-pushed the mockable-sleep-#228 branch from f78368a to 9fa1280 Compare September 11, 2020 08:35
@asqui
Copy link
Contributor Author

asqui commented Sep 11, 2020

Ok, I think this branch is ready to merge now.

@asqui
Copy link
Contributor Author

asqui commented Oct 9, 2020

@ivanprado : Fascinating failure when I merged your change 78a462e (#253) into this PR branch.

This PR makes a tweak to allow easier mocking of nap.sleep, but the test failures indicate that when using the Retrying.call() method (which is deprecated by your commit) sleep() is now getting called 24 times rather than 4.

I tweaked the test to invoke using __call__ (i.e. retrying(fn) rather than retrying.call(fn) in the spirit of your change) and this resolves the issue, but it suggests that something is very wrong with the backward compatibility for call() introduced by your commit.

Smells like we're getting nested layers of retrying, but I can't immediately see how or why that is happening. (Also, if that were the case I'd intuitively expect 4 x 4 = 16 retries, but seems we're getting 24 (= 4 x 6?) retries.)

This failure also hints we might want to (re-)introduce test coverage for the Retrying.call() method.

@ivanprado
Copy link
Contributor

Wow. I have no idea what can be going on.

To give an idea, we might change this line 78a462e#diff-03f4d209c0b66034a81979dd67efaa24R435 by:

self(self, *args, **kwargs)

But in theory, it is completely equivalent. Could you try just in case it changes anything?

@asqui
Copy link
Contributor Author

asqui commented Oct 20, 2020

@ivanprado : The issue was that you are passing self as the first argument, which is meant to be the function to be executed by the retryer. That is, the retryer is going to execute itself, with retries!

PR #258 raised to resolve this.

@jd
Copy link
Owner

jd commented Oct 22, 2020

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 22, 2020

Command update: success

Branch has been successfully updated

@asqui
Copy link
Contributor Author

asqui commented Oct 22, 2020

Tests are green again now.

Is there anything else required before this PR can be merged?

@mergify mergify bot merged commit f079280 into jd:master Oct 24, 2020
@asqui asqui deleted the mockable-sleep-#228 branch October 25, 2020 15:52
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.

Cannot easily mock out sleep function in order to simulate time in tests
3 participants