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: test_tools's mocks are obsolete #1781

Merged
merged 2 commits into from
Dec 15, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Dec 9, 2019

This PR makes test_tools's mocks obsolete, adding deprecation warning on MockConfig, MockSopel, and MockSopelWrapper - replaced by the configfactory fixture, the botfactory fixture, and by the sopel.bot.SopelWrapper class respectively.

As far as I'm concerned, Sopel's codebase doesn't use MockSopel nor MockConfig, making sure that no part of Sopel assumes ~/.sopel/ as the homedir. We may close issue #946 once this is merged.

@dgw
Copy link
Member

dgw commented Dec 9, 2019

I'm not a huge fan of needing to put $nickname: into every example that uses bot.reply(). 🙁

Dropping test_tools without warning isn't entirely safe, FYI. It's not in the API docs, but it is referenced in our help tutorial. That's basically "documented", and a quick code search of GitHub for "sopel test_tools" shows a number of third-party packages that use our test tools for their own plugin tests.

@Exirel
Copy link
Contributor Author

Exirel commented Dec 10, 2019

Ok, I'll do two thing:

  • be more flexible with $nickname (and revert my changes about that)
  • add some deprecation warnings here and there

And I think we'll be good to go. As mentioned on IRC, internal plugins will become externals by Sopel 8.0, and we have a year to craft better testing tools. I'll remember that:

09:45:20 <+dgw> (I would love a testing framework that just auto-tests plugins without having to do the __name__ == 'main' crap though :D)

@Exirel
Copy link
Contributor Author

Exirel commented Dec 10, 2019

@dgw and done! It's way easier to review now I think.

@Exirel Exirel marked this pull request as ready for review December 10, 2019 13:20
@Exirel Exirel requested a review from dgw December 10, 2019 13:20
@Exirel Exirel added this to the 7.0.0 milestone Dec 10, 2019
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.

Minor things. Deprecating the Mock* classes is a natural thing to do after making the "real" objects testable directly. We can keep the discussion going about how to test say() vs. reply() in a backwards-compatible way, if you like. 😸

sopel/test_tools.py Outdated Show resolved Hide resolved
sopel/test_tools.py Outdated Show resolved Hide resolved
test/modules/test_modules_remind.py Show resolved Hide resolved
test/test_db.py Show resolved Hide resolved
@dgw dgw added the Tests label Dec 11, 2019
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.

Squash what you're gonna squash, @Exirel.

@dgw
Copy link
Member

dgw commented Dec 12, 2019

@Exirel Oh wait, this still has a "post-review fix" commit that should be squashed out before merge. I guess you can take care of that and glance at #1777 at the same time. 😁

@Exirel
Copy link
Contributor Author

Exirel commented Dec 12, 2019

lol

@Exirel
Copy link
Contributor Author

Exirel commented Dec 12, 2019

Done.

dgw added a commit that referenced this pull request Dec 15, 2019
@dgw dgw merged commit ba9bf84 into sopel-irc:master Dec 15, 2019
@Exirel Exirel deleted the test-remove-mocksopel-class branch January 14, 2020 15:00
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