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

Clean up Invoicing Tests #35576

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

Clean up Invoicing Tests #35576

wants to merge 8 commits into from

Conversation

nospame
Copy link
Contributor

@nospame nospame commented Jan 3, 2025

Technical Summary

Reorganizes and lightly refactors invoicing test suites. Specifically removes TestDomainInvoiceFactory because most of that functionality is already covered in TestInvoice, and in review here it became clear that the distinction was blurry and likely unnecessary.

Review by commit.

Safety Assurance

Safety story

Changes automated tests only, no change to functional code.

Automated test coverage

It's automated tests all the way down.

QA Plan

Nope.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@nospame nospame requested review from mjriley and jingcheng16 January 3, 2025 17:30
@nospame nospame marked this pull request as ready for review January 3, 2025 17:30
@nospame nospame added the product/invisible Change has no end-user visible impact label Jan 3, 2025
is_testing_web_user_feature = False

@classmethod
def setUpClass(cls):
Copy link
Member

Choose a reason for hiding this comment

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

move to setUp instead of setUpClass, and likewise with tearDownClass

Copy link
Member

Choose a reason for hiding this comment

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

(additionally, better to move tearDown functionality to cleanup classes, so that cleanup actually executes if an error occurs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After working on this for a couple hours, it's looking pretty messy to try and move the subscription setup entirely out of the class method, based on various subclasses assuming (reasonably) that there is always a default subscription.

Rather than explicitly assigning subscriptions in each of those subclasses, I may end up instead, for test cases that need a unique subscription (rather than the default), explicitly replacing that default subscription instead of adding a new one (which I believe was @jingcheng16 's concern in test_community_invoice, that we were just generating a new subscription but leaving still leaving the old one intact)

Copy link
Contributor Author

@nospame nospame Jan 9, 2025

Choose a reason for hiding this comment

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

Actually, that wouldn't quite work either, because then further test cases would be missing the expected class-subscription. It's specifically the TestBillingAutoPay classes that make heavy use of the class-subscription, so maybe I can restructure those a bit to work with a subscription that gets set per-test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants