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.

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

Enhance: Introducing tests #269

Merged
merged 7 commits into from
Apr 25, 2023
Merged

Enhance: Introducing tests #269

merged 7 commits into from
Apr 25, 2023

Conversation

nautics889
Copy link
Contributor

@nautics889 nautics889 commented Apr 18, 2023 •
edited
Loading

I'd like to keep it as a draft for a while if you don't mind.
I appreciate if you share any proposal or feedback regarding tests for further updates in the scope of this PR.

@nautics889
Enhance: Introducing tests
a6c2bc3 
Initialized tests (for pytest runner).
Initialized tests for `BasePixivAPI`.
Add test case for `request_call` of `BasePixivAPI` using GET HTTP-method.
@nautics889
Enhance: Introducing tests
46de313 
Add tests for different HTTP-methods for `request_call` of
`BasePixivAPI` class.
Add tests for `additional_headers()`, `parse_json()` methods.
@nautics889
Enhance: Introducing tests
819c697 
Add utils for reading fixtures data from files.
Add fixtures containing common valid JSON data and invalid JSON data.
@nautics889
Enhance: Introducing tests
50dbeb6 
Add tests for `download()` method of `BasePixivAPI` class.
@nautics889
Enhance: Introducing tests
d860de3 
Add workflow job for running tests with pytest.
Add configfile for pytest (pytest.ini).
@nautics889
Enhance: Introducing tests
c2619df 
Update extra dependencies order in setup.cfg.
Update imports order in tests/utils.py.
@nautics889
Enhance: Introducing tests
2b84950 
Fix linter issue (redundant new lines) in setup.cfg, tests/utils.py.
@nautics889
Copy link
Contributor Author

inb4:

@nautics889
Copy link
Contributor Author

Before continue covering by tests i'd like to open PR at this point to consider your comments and suggestions. And if everything ok i'm up to merge that.
@upbit, @eggplants could you please take a quick glance when you find time?

@nautics889 nautics889 marked this pull request as ready for review April 23, 2023 22:16
@upbit
Copy link
Owner

upbit commented Apr 23, 2023

Good job! I had also thought about introducing tests, but considering the need for a showcase for API, I just used demo.py and output response (actually, I was lazy...). However, the actual situation is that the pipeline will be limited by Pixiv's instability.

Introducing pytest is great, but I don't have much practical experience in this area. You can refer to the best practices of pytest, or wait for others reviews to take a look. (@Xdynix )

In addition, for the actual unit test part, it is recommended to use data-driven testing (or table-driven testing), make it easy to add test cases later.

@upbit upbit requested review from Xdynix and upbit April 23, 2023 23:28
Xdynix
Xdynix approved these changes Apr 23, 2023
View reviewed changes
Copy link
Collaborator

@Xdynix Xdynix 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 to me. Please squash the commits when merging to keep the git log clean.

upbit
upbit approved these changes Apr 23, 2023
View reviewed changes
Copy link
Owner

@upbit upbit left a comment

Choose a reason for hiding this comment

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

LGTM

@nautics889
Copy link
Contributor Author

Thanks for review guys!

@upbit should i squash the commits by myself (with git rebase, squash and then force-push) or you're going to do this when merging the PR ("Squash and Merge" option)?

@upbit
Copy link
Owner

upbit commented Apr 25, 2023

@upbit should i squash the commits by myself (with git rebase, squash and then force-push) or you're going to do this when merging the PR ("Squash and Merge" option)?

Actually, it's not necessary. Before merging MR to master, just choose Squash and merge.

@nautics889
Copy link
Contributor Author

nautics889 commented Apr 25, 2023 •
edited
Loading

@upbit sorry for disturbing you again, could you please hit "Squash and merge" if this PR looks OK in your opinion at current stage? Cause i can't merge the PR since i don't have write access to the repo.
Screenshot 2023-04-25 165005

And then i'd continue to enhance test coverage, in other PR's (i'd suggest to make one PR per test case/test suite).

@upbit upbit merged commit aec177a into upbit:master Apr 25, 2023
@upbit
Copy link
Owner

upbit commented Apr 25, 2023

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xdynix Xdynix Xdynix approved these changes

@upbit upbit upbit approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nautics889 @upbit @Xdynix