Skip to content

Refactor: split unit tests by API resources #1078

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 26, 2020
Merged

Refactor: split unit tests by API resources #1078

merged 4 commits into from
Aug 26, 2020

Conversation

nejch
Copy link
Member

@nejch nejch commented Apr 17, 2020 •
edited
Loading

I've been thinking about how to make testing more accessible for contributors and maybe splitting them by resources in the Gitlab API docs would be best because that's how most people interact with this anyway. If there's a new API resource it likely gets a new page so this would kind of make it a 1-to-1 for new contributions.

Ideas:

  • Split unit tests by API resources
  • Convert unittest classes into modules? (disclaimer: really biased here against classes :P)
  • Figure out a better way for fixtures & mocking copy/pasting

max-wittig
max-wittig reviewed Apr 17, 2020
View reviewed changes
max-wittig
max-wittig reviewed Apr 17, 2020
View reviewed changes
Copy link
Member

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Looks good overall so far!

I'm also biased against classes in python. See also the last sentence of the Zen of Python

>>> import this
The Zen of Python, by Tim Peters

Beautiful is better than ugly.
Explicit is better than implicit.
Simple is better than complex.
Complex is better than complicated.
Flat is better than nested.
Sparse is better than dense.
Readability counts.
Special cases aren't special enough to break the rules.
Although practicality beats purity.
Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
Now is better than never.
Although never is often better than *right* now.
If the implementation is hard to explain, it's a bad idea.
If the implementation is easy to explain, it may be a good idea.
Namespaces are one honking great idea -- let's do more of those!
>>>

@nejch nejch force-pushed the refactor/split-unit-tests branch from 3cbafae to f1ebb88 Compare April 25, 2020 10:24
@nejch
Copy link
Member Author

nejch commented Apr 25, 2020

Looks good overall so far!

I'm also biased against classes in python. See also the last sentence of the Zen of Python

😁 that's great I'm glad, I was afraid these changes would be too much. I've only left the 2 classes in test_base.py for now as it's a small file and didn't seem to make sense to split it yet.

I realized I might be getting myself into a continuous rebase party as new PR's with tests in the old files are merged, so I've cleaned it up a bit more and I think I want to look at fixtures in a later PR when I have more time, if this is in an OK shape?

@nejch nejch marked this pull request as ready for review April 25, 2020 10:45
@nejch nejch requested a review from max-wittig April 25, 2020 10:45
@max-wittig
Copy link
Member

@nejch Sorry totally missed this. Does one only see assignments and not reviewers in the feed?

@max-wittig
Copy link
Member

@nejch I would love to merge this. Could you please rebase to latest master?

@nejch nejch force-pushed the refactor/split-unit-tests branch from f1ebb88 to 9edf10f Compare August 23, 2020 19:17
@nejch nejch force-pushed the refactor/split-unit-tests branch from 9edf10f to 204782a Compare August 23, 2020 19:20
@nejch
Copy link
Member Author

nejch commented Aug 23, 2020

@nejch I would love to merge this. Could you please rebase to latest master?

Sorry @max-wittig I'm back from the dead/vacation/other distractions now 😁 I've rebased and rewritten the tests using responses now so it's more consistent with your runners example. Could you have another look to see if this is how you imagined responses-based tests?

Not sure about notifications, there's a separate Notification setting for PR Reviews so maybe you had them turned off?

@nejch nejch requested a review from max-wittig August 23, 2020 19:50
@max-wittig
Copy link
Member

max-wittig commented Aug 24, 2020 •
edited
Loading

We actually have less tests with this MR. I guess some got lost. Could yout take a look? @nejch

155 vs. 158

This MR: https://travis-ci.org/github/python-gitlab/python-gitlab/jobs/720445167
Master: https://travis-ci.org/github/python-gitlab/python-gitlab/jobs/720460657

@nejch
Copy link
Member Author

nejch commented Aug 25, 2020

We actually have less tests with this MR. I guess some got lost. Could yout take a look? @nejch

155 vs. 158

This MR: https://travis-ci.org/github/python-gitlab/python-gitlab/jobs/720445167
Master: https://travis-ci.org/github/python-gitlab/python-gitlab/jobs/720460657

Oof, I was weirded out by this until I realized that on master, 2 test cases (in test_groups.py) were run 3 times. They're duplicates because I had accidentally put them in a unittest base class (TestGroup) inherited by other test classes back when I was adding group export/import tests. 🤦
With pytest they're run only once, hence the lower number. If you exclude test_groups.py the number should be higher. But I checked all cases collected in master vs. this PR again and it should be OK otherwise.

With tox -e py38 -- -v:

master:

gitlab/tests/objects/test_groups.py::TestGroup::test_create_group PASSED [ 75%]
gitlab/tests/objects/test_groups.py::TestGroup::test_get_group PASSED    [ 75%]
gitlab/tests/objects/test_groups.py::TestGroupExport::test_create_group PASSED [ 76%]
gitlab/tests/objects/test_groups.py::TestGroupExport::test_create_group_export PASSED [ 76%]
gitlab/tests/objects/test_groups.py::TestGroupExport::test_download_group_export PASSED [ 77%]
gitlab/tests/objects/test_groups.py::TestGroupExport::test_get_group PASSED [ 78%]
gitlab/tests/objects/test_groups.py::TestGroupExport::test_refresh_group_export_status SKIPPED [ 78%]
gitlab/tests/objects/test_groups.py::TestGroupImport::test_create_group PASSED [ 79%]
gitlab/tests/objects/test_groups.py::TestGroupImport::test_get_group PASSED [ 80%]
gitlab/tests/objects/test_groups.py::TestGroupImport::test_import_group PASSED [ 80%]
gitlab/tests/objects/test_groups.py::TestGroupImport::test_refresh_group_import_status SKIPPED [ 81%]

refactor:

gitlab/tests/objects/test_groups.py::test_get_group PASSED               [ 56%]
gitlab/tests/objects/test_groups.py::test_create_group PASSED            [ 56%]
gitlab/tests/objects/test_groups.py::test_create_group_export PASSED     [ 57%]
gitlab/tests/objects/test_groups.py::test_refresh_group_export_status SKIPPED [ 57%]
gitlab/tests/objects/test_groups.py::test_download_group_export PASSED   [ 58%]
gitlab/tests/objects/test_groups.py::test_import_group PASSED            [ 58%]
gitlab/tests/objects/test_groups.py::test_refresh_group_import_status SKIPPED [ 59%]

@nejch nejch assigned nejch and max-wittig and unassigned nejch Aug 25, 2020
@max-wittig max-wittig merged commit a7e44a0 into master Aug 26, 2020
@max-wittig
Copy link
Member

Thank you very much! Very nice!

@max-wittig max-wittig deleted the refactor/split-unit-tests branch August 26, 2020 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@max-wittig max-wittig Awaiting requested review from max-wittig

Assignees

@max-wittig max-wittig

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nejch @max-wittig