Skip to content

Refactor test cases to improve unit test quality #5986

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

Merged
merged 3 commits into from
Jul 26, 2023

Conversation

freddiewanah
Copy link
Contributor

Hello, first-time submitting PR here.

This PR improves the readability of the assert statements in some of the unit tests by using more expressive assert methods from the unittest module. These changes improve code quality (avoid test smells) by:

  • Making the assert statements more readable, e.g., the use of assertTrue, assertFalse, assertRaises.

  • Simplifying code management. For example, instead of simply saying "this", use the parameter name(UserDefinedFilesPipeline.DEFAULT_FILES_RESULT_FIELD) so that developers could only update the value in one place.

Happy to update more related issues in the test code if needed :)

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #5986 (858ce04) into master (af2aa4b) will not change coverage.
The diff coverage is n/a.

❗ Current head 858ce04 differs from pull request most recent head 389da53. Consider uploading reports for the commit 389da53 to get more accurate results

@@           Coverage Diff           @@
##           master    #5986   +/-   ##
=======================================
  Coverage   89.21%   89.21%           
=======================================
  Files         162      162           
  Lines       11301    11301           
  Branches     1835     1835           
=======================================
  Hits        10082    10082           
  Misses        927      927           
  Partials      292      292           

@freddiewanah
Copy link
Contributor Author

Hi wRAR, thanks for the comments. I've reverted the updates about all the assertRaises.

@freddiewanah
Copy link
Contributor Author

Update the assert format with black.

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Nice cleanup, thanks!

@Gallaecio Gallaecio merged commit c4f0aa4 into scrapy:master Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants