-
Notifications
You must be signed in to change notification settings - Fork 295
Add RST support in Sandbox #2917
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
base: master
Are you sure you want to change the base?
Conversation
Added RST test label files as resources. Added a RstTmchUtils class that loads appropriate labels according to TLD pattern. Temporarily changed label fetching in production to include the TLD string, so that the new class may know which set of labels to use.
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_production(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getClaimsList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
| @ParameterizedTest | ||
| @MethodSource("provideTestCases") | ||
| @SuppressWarnings("unused") // testCaseName | ||
| void getSmdrList_sandbox(String testCaseName, String tld) { |
Check notice
Code scanning / CodeQL
Useless parameter Note test
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 resolved 8 discussions.
Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @CydeWeys and @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 reviewed 12 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CydeWeys and @weiminyu).
core/src/test/java/google/registry/tmch/RstTmchUtilsTest.java line 39 at r2 (raw file):
try { PRODUCTION.setup(); assertThat(getClaimsList(tld)).isEmpty();
shouldn't we verify that in production we return the default claims list (that we'd have to create in a before method)?
same question for smdrl
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
private static final FluentLogger logger = FluentLogger.forEnclosingClass(); enum RstType {
this seems odd, why do we have to distinguish between these two if we're running all of this in the sandbox (OTE) environment? is that a difference on their end?
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 63 at r2 (raw file):
/** * Returns appropriate test labels if {@code tld} is for RST testing; otherwise returns {@code * defaultList}.
i don't think "defaultList" is actually referenced anywhere as such (same with the method below)
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 90 at r2 (raw file):
} private static Optional<ClaimsList> getClaimList(RstType rstType) {
nit: getClaimsList (with an s)
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 98 at r2 (raw file):
try { return Optional.of(ClaimsListParser.parse(readLines(resource, UTF_8))); } catch (IOException fnfe) {
hah, what's up with the exception variable name here? (same in the next method too)
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 made 3 comments and resolved 1 discussion.
Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
Previously, gbrodman wrote…
this seems odd, why do we have to distinguish between these two if we're running all of this in the sandbox (OTE) environment? is that a difference on their end?
Renamed enum class name. These refers to RST's ote vs prod.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 63 at r2 (raw file):
Previously, gbrodman wrote…
i don't think "defaultList" is actually referenced anywhere as such (same with the method below)
Done.
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 98 at r2 (raw file):
Previously, gbrodman wrote…
hah, what's up with the exception variable name here? (same in the next method too)
Done.
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 reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @weiminyu).
core/src/main/java/google/registry/tmch/RstTmchUtils.java line 47 at r2 (raw file):
Previously, weiminyu (Weimin Yu) wrote…
Renamed enum class name. These refers to RST's ote vs prod.
got it, so this is all just entirely on their end thx
Added RST test label files as resources.
Added a RstTmchUtils class that loads appropriate labels according to TLD pattern.
Temporarily changed label fetching in production to include the TLD string, so that the new class may know which set of labels to use.
This change is