tests: MockIRCServer methods should block by default
#2065
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`factories.BotFactory` had an outdated example (old method name). `pytest_plugin.ircfactory` contained a syntax error (typo) in its example. `mocks.MockIRCServer.channel_joined` had a spelling error (typo).
With 30 new lines of code (of which only about half is actual code), a solid 20 lines can be removed from the `isup` tests. The long-term benefit is that there will be less boilerplate required when writing tests for plugin callables that are threaded (which is the default value if the `@plugin.thread` decorator doesn't override it).
dgw
force-pushed
the
auto-join-threads-MockIRCServer
branch
from
1f8847a
to
297b71b
Compare
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
tl;dr: Threaded callables can cause race conditions during tests, but the
MockIRCServeris in a perfect position to make sure they don't (and it now does so).I had a very interesting time this past weekend, trying to write tests for #2063 that simply wouldn't work, no matter what I did, until eventually the idea came to me: "Try it with the plugin callable set to unthreaded." After that, and an IRC discussion wherein @Exirel reminded me that the
bot.running_triggersproperty exists, those tests got written and merged.Now this new feature inspired by the above process is ready to be "formally" proposed. I spent some time on the documentation for it, proved the concept by running the test suite on three local Python versions (2.7, 3.3, and 3.8), and am reasonably confident that this is ready for the "bikeshedding" or "nitpicking" stage—that is, developed enough as a concept that I won't need to rewrite the whole patch based on feedback. 😉
I elected to go with the simpler Boolean argument value for the overriding (
blocking) argument on each method. Also proposed was a numeric value that would allow a test writer to specify thetimeoutforthreading.Thread.join(), but that seems needlessly complex. I can't see a use for tests where a call would sometimes block and sometimes time out—such a test would be prone to intermittent failures, and thus not very useful for CI. 😁Checklist
make qa(runsmake qualityandmake test)Notes
Of course, if I've made any silly assumptions about things like never needing to set a
timeoutonthreading.Thread.join()sometimes, that's what the PR discussion is for!The
versionaddeddirectives are committed separately, so I can easily update them if for some reason this doesn't go into 7.1. Otherwise, I plan to squash them into the relevant code commits before merging.