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

Enhancement: Refactor Test Code for Efficiency and Quality #1728

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

freddiewanah
Copy link

Description

As a first-time contributor with a background in researching Python unit tests, I conducted a thorough static analysis of the project's test code. This examination revealed various "test smells" that could potentially degrade both the efficiency and quality of our tests. To address these issues, I've refactored the relevant code segments. The modifications I propose not only aim to eliminate these inefficiencies but are also expected to reduce execution time within GitHub Actions. This reduction could contribute to lower operational costs for the project.

Test smells refactored:

  • Suboptimal Assert: Instead of using statements such as assertIsNone, assertIsInstance, always simply use assertTrue or assertFalse. This will decrease the code overall readability and increase the execution time as extra logic needed.
  • Duplicate Assert: Having same asserts in one test case. Having multiple asserts in one test cases can cause potential issues like when the first assert fails, the test case stops and won't check the rest of the asserts. By using @parameterized.expand, this issue can be resolved and the caching also saves execution time.
  • Assert in SetUp: "Using assertions in the SetUp method leads to redundant execution: The SetUp method is called before each test method within the same class runs. As a result, any assertions placed in SetUp are executed multiple times—once for each test method. To refactor, I've extracted the asserts into a separate test method and make sure they are only triggered once per time.
  • Unknown test: A test case without assertions. Having no assertions in the test case may increase the difficulty in understanding the test case and throw different error messages when it fails.
  • Magic Number: Using numeric literals (known as 'magic numbers') directly in assert statements makes the test cases less maintainable and harder to understand. Instead of relying on these opaque values, refactoring to use descriptive constants or variables enhances clarity and maintainability. Additionally, this approach avoids the overhead of introducing new parameters by leveraging caching, which can theoretically save execution time.

I didn't change any of the testing logic or testing values. The refactoring only focuses on reducing the test smells.

Please feel free to let me know if you are interested in more refactoring like this or if there're any changes that you don't wish.

@anadrianmanrique anadrianmanrique added the high High priority item label Apr 18, 2024
@anadrianmanrique
Copy link
Contributor

@freddiewanah: Thanks for the amazing work you've done and also for providing such an excellent context explaining the changes!
I will start working on improvements in testing by the end of May/first days of June. Validating this PR is my TODO list now.
If you are interested in improving the test suit we can discuss which tasks are priority for the next release. Let me know your thoughts ( including which other tasks you see as a priority ).
Thanks!!

@freddiewanah
Copy link
Author

Hi anadrianmanrique, thanks for prioritizing this PR.

I'm mainly a researcher who works in detecting and refactoring test smells in Python projects. I think two common issues many open-source Python projects are facing are having too many assertions in one test case that will make them mega test cases and not utilizing the decorations provided by testing frameworks. If you are interested in these directions, I could work on this direction and contribute more.

Signed-off-by: Han Wang <freddie.wanah@gmail.com>
@freddiewanah
Copy link
Author

I'm sorry about the assert error. It has been fixed and should have no errors.

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