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

test: botfactory, UserFactory, and IRCFactory #1731

Merged
merged 4 commits into from
Nov 22, 2019
Merged

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Oct 31, 2019

I wanted more from Sopel's test tools, for 2 reasons:

  • increase the test coverage for the bot, its internals and its plugins
  • help plugin authors to test their plugins

My approach in this PR is the following:

  • sopel.tests.mocks contains mocks: they generate fake IRC lines (User and IRC Server) or pretend to be a backend of some sort (IRC Connection Backend)
  • sopel.tests.factories contains factories: they are helper classes, they help to create mock objects with different parameters. Each factory implements a __call__ method, so they act as callable, like this: mock_bot = botfactory(...)
  • sopel.tests.pytest_plugin: it's a plugin for pytest, that will be automatically loaded by pytest thanks to entry point. It will bring shared fixture for everyone to use, mostly generating pre-made factories

@Exirel Exirel added this to the 7.0.0 milestone Oct 31, 2019
@Exirel Exirel requested a review from dgw October 31, 2019 21:23
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Aside from notes in the docstrings, I'm not sure we want to start calling things "Factory". I know you're not set on the naming yet; just weighing in! Personally, I would write everything as MockSomething, and have them all be classes (including botfactory) for consistency, but that's just one idea.

sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Nov 14, 2019

Ok, now I think the interface works well enough. The next step is to squash the commits.

To move forward with tests and their tools, I'll need #1732 to be merged with that PR, so I will be able to run a command, join the command thread, then check the bot's output.

@dgw awaiting your approval to rebase/squash. :)

@Exirel
Copy link
Contributor Author

Exirel commented Nov 19, 2019

@dgw after so many changes in master (about ~100 commits of differences) I had to rebase, so I also squashed the previous commits. I know it's a big chunk of code to review. The good news is that it doesn't change the core of Sopel, and affect only the tests. Now with #1732 I should be able to write more complex tests, that will verify that the bot works from its dispatch method to the last bit of coretasks.

Which is pretty nice in my opinion and opens a ton of possibilities for the future. 🕺

@Exirel Exirel force-pushed the test-tools branch 2 times, most recently from b18150e to a47c7d5 Compare November 20, 2019 22:03
@Exirel
Copy link
Contributor Author

Exirel commented Nov 20, 2019

So for some reason I removed the import re in the wrong commit, so my squash didn't result as intended. Now it's fixed properly, I won't get caught in that again!

@Exirel
Copy link
Contributor Author

Exirel commented Nov 20, 2019

Yet another "sopel/modules/search.py::test_example_duck_1 FAILED". Go to hell, DDG.

@dgw
Copy link
Member

dgw commented Nov 20, 2019

Go to hell, DDG.

Big mood. 😂

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Phew! Surprisingly, I got through this in just over half an hour.

Have your daily (?) dose of typo fixes, a healthy helping of consistency tweaks, and a couple honest questions for dessert. I really like how much this simplifies the tests, even if the diff doesn't look like it (net +419 lines overall, but the actual unit test files are net negative).

sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
test/test_bot.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Outdated Show resolved Hide resolved
@Exirel Exirel requested a review from dgw November 21, 2019 11:48
@Exirel
Copy link
Contributor Author

Exirel commented Nov 21, 2019

@dgw I didn't rebase/squash, you shouldn't have trouble with the new review.

I think this is in a good shape - good enough for a release - but I certainly can't wait for all the feedbacks that will help improve and build on what's in here.

Kind of excited for the future right now! 🎉

@Exirel
Copy link
Contributor Author

Exirel commented Nov 21, 2019

Coverage increased (+1.3%) to 52.652%

Me: 💥 👶

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I'm approving this despite a couple little formatting things, which you will probably handle during squash. If they aren't taken care of now, they'll be handled later by flake8 checks I plan to add after 7.0.

sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/tests/mocks.py Outdated Show resolved Hide resolved
test/test_coretasks.py Show resolved Hide resolved
test/test_loader.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Nov 21, 2019

💥 👶 😎

@dgw dgw merged commit 36b9685 into sopel-irc:master Nov 22, 2019
@Exirel Exirel deleted the test-tools branch January 14, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants