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

loader: test coverage #1424

Merged
merged 8 commits into from
Jan 31, 2019
Merged

loader: test coverage #1424

merged 8 commits into from
Jan 31, 2019

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Dec 27, 2018

The sopel.loader module is responsible for loading Sopel Module (as in, "plugins" for Sopel), so it's quite critical.

So I added unit-tests, and change as few code as possible.

There is one function that I didn't test, because I want to rework it later: enumerate_modules. This function does too many things, and relies on too many global or hard-coded variables. This must be changed.

So, for now, here is a good increase in coverage for the project.


Initial description:

Hi! It's a WIP and I don't expect it to be merged "as-is". I wanted to dive into Sopel's code, just to give it a try, and found that I could help on the test side of thing.

I think I can do plenty more on the sopel.loader module, but I wanted to share this as soon as possible, to see if it would help and/or was in the right direction, code-style wise in particular.

I'll work on this in the following days, but don't hesitate to stop me, ask question, make remarks, etc. feedback is very welcome!

@Exirel
Copy link
Contributor Author

Exirel commented Dec 27, 2018

I'll fix CI build tomorrow, I forgot to check flake8, my bad!

Edit: done! Resume testing... 😄

@Exirel Exirel force-pushed the coverage-sopel-loader branch 2 times, most recently from 38b9608 to d6b5022 Compare December 27, 2018 17:31
@dgw dgw added this to the 7.0.0 milestone Jan 3, 2019
@dgw dgw changed the title [WIP] Coverage sopel loader loader: test coverage Jan 3, 2019
@dgw
Copy link
Member

dgw commented Jan 3, 2019

Just took a peek because I was curious (and noticed I hadn't labeled this yet). The most exciting thing I see is Coveralls' current check status: "Coverage increased (+1.06%) to 43.248%" 🎉

@Exirel Exirel mentioned this pull request Jan 3, 2019
@dgw
Copy link
Member

dgw commented Jan 5, 2019

@Exirel I just found a really old (last edited in June 2018) test_loader.py in one of my local repos. You've probably already moved beyond it (and it's a cringey mess, anyway), but in case any of it is useful: here. 😸

@Exirel
Copy link
Contributor Author

Exirel commented Jan 5, 2019

@dgw I need to check that. There are a lot of interesting things in there!

@Exirel Exirel force-pushed the coverage-sopel-loader branch from d6b5022 to 4300b06 Compare January 10, 2019 17:36
@Exirel Exirel force-pushed the coverage-sopel-loader branch from 4300b06 to 4acb6b5 Compare January 23, 2019 19:33
@Exirel
Copy link
Contributor Author

Exirel commented Jan 23, 2019

@dgw you can remove the WIP tag and have a look. I bet you can merge that one eyes closed. 😉

@Exirel Exirel force-pushed the coverage-sopel-loader branch from 0856072 to c9b749b Compare January 24, 2019 09:10
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 really expected to put an Approve on this immediately, but I guess there are a couple things I'd like to see changed after all.

It's really thorough overall, and I like where it's at. It's really almost there!

test/test_loader.py Outdated Show resolved Hide resolved
test/test_loader.py Show resolved Hide resolved
test/test_loader.py Outdated Show resolved Hide resolved
sopel/loader.py Outdated Show resolved Hide resolved
Tested cases:

* test function has new attributes,
* these attributes all have a default value,
* test the `commands` and `nickname_commands` attribute,
* test the `rule` attribute,
* test the `event` atribute,

Missing cases are:

* doc,
* and example.

Still better than nothing!
@Exirel Exirel force-pushed the coverage-sopel-loader branch from c9b749b to ee32151 Compare January 30, 2019 10:37
@Exirel
Copy link
Contributor Author

Exirel commented Jan 30, 2019

It's really thorough overall, and I like where it's at. It's really almost there!

Should be better now @dgw and thanks for the review again!

Edit: apparently not. I forgot something somewhere... :|

The config's default value for "nick" could be changed, breaking any
tests that rely on this value. To prevent that, the tmpconfig fixture
now provides a test nick.
@Exirel Exirel force-pushed the coverage-sopel-loader branch from ee32151 to 2895155 Compare January 30, 2019 11:00
@dgw dgw merged commit e7ab1c3 into sopel-irc:master Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants