-
Notifications
You must be signed in to change notification settings - Fork 295
Disable old fee extensions in non-prod envs #2933
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
Conversation
core/src/test/java/google/registry/flows/domain/DomainCreateFlowOldFeeExtensionsTest.java
Fixed
Show fixed
Hide fixed
305964d to
c45224d
Compare
|
the way this works: Basically, we change the XML loaders so that in a non-production environment they only load the fee 1.0 extension, and in production environments we only load the fee-0.x extensions. This totally messes up all the tests that use XML and load fee extensions. We split each of the Domain*FlowTest classes that uses the fee extension into two classes -- one that runs against a faked production environment and uses the fee-0.x extensions and the standard test file which runs against the fee-1.0 extension (in the unit test environment, which is the default). We move all tests that use the old fee extensions into the production-simulating test classes and duplicate each test in the main class using the 1.0 extension. This does add tests in general, though we should be able to remove the production-simulating tests eventually. |
c45224d to
ade1b3e
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there have been a way to do this more simply by making the loader UNITTEST-aware? I.e. don't remove the draft versions of the fee extension except in sandbox, and then just ... don't test specifically test that configuration? Just have normal individual tests for each version of the fee extension?
@CydeWeys made 1 comment.
Reviewable status: 0 of 120 files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java
Outdated
Show resolved
Hide resolved
...st/resources/google/registry/flows/domain/domain_check_fee_response_thirty_domains_stdv1.xml
Outdated
Show resolved
Hide resolved
ade1b3e to
4db238f
Compare
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 1 comment.
Reviewable status: 0 of 134 files reviewed, 3 unresolved discussions (waiting on @gbrodman).
4db238f to
c7acd19
Compare
weiminyu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiminyu partially reviewed 134 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gbrodman).
gbrodman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbrodman made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys).
core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java
Outdated
Show resolved
Hide resolved
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gbrodman).
core/src/test/java/google/registry/flows/domain/DomainCheckFlowTest.java
Outdated
Show resolved
Hide resolved
CydeWeys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CydeWeys resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gbrodman).
The primary annoyance with this is that it means we need (or at least, should) split all tests that use the fee extension into two separate tests -- one that simulates non-prod environments, and one that simulates prod environments. This leads to duplication of many tests but that's fine since this is theoretically temporary.
c7acd19 to
367638f
Compare
The primary annoyance with this is that it means we need (or at least, should) split all tests that use the fee extension into two separate tests -- one that simulates non-prod environments, and one that simulates prod environments. This leads to duplication of many tests but that's fine since this is theoretically temporary.
This change is