Skip to content

Conversation

@weiminyu
Copy link
Collaborator

@weiminyu weiminyu commented Dec 30, 2025

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 Reviewable

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

The parameter 'testCaseName' is never used.
@ParameterizedTest
@MethodSource("provideTestCases")
@SuppressWarnings("unused") // testCaseName
void getSmdrList_production(String testCaseName, String tld) {

Check notice

Code scanning / CodeQL

Useless parameter Note test

The parameter 'testCaseName' is never used.
@ParameterizedTest
@MethodSource("provideTestCases")
@SuppressWarnings("unused") // testCaseName
void getClaimsList_sandbox(String testCaseName, String tld) {

Check notice

Code scanning / CodeQL

Useless parameter Note test

The parameter 'testCaseName' is never used.
@ParameterizedTest
@MethodSource("provideTestCases")
@SuppressWarnings("unused") // testCaseName
void getSmdrList_sandbox(String testCaseName, String tld) {

Check notice

Code scanning / CodeQL

Useless parameter Note test

The parameter 'testCaseName' is never used.
Copy link
Collaborator Author

@weiminyu weiminyu left a 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).

Copy link
Collaborator

@gbrodman gbrodman left a 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)

Copy link
Collaborator Author

@weiminyu weiminyu left a 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.

Copy link
Collaborator

@gbrodman gbrodman left a 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

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.

2 participants