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

api-common-java/src/test/java/com/google/api/pathtemplate/PathTemplateTest.complexResourceIdInvalidDelimiter test erroneously succeeding #2776

Closed
alicejli opened this issue May 15, 2024 · 1 comment · Fixed by #2823
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@alicejli
Copy link
Contributor

alicejli commented May 15, 2024

https://github.com/googleapis/sdk-platform-java/blob/v2.40.0/api-common-java/src/test/java/com/google/api/pathtemplate/PathTemplateTest.java#L356

@test
public void complexResourceIdInvalidDelimiter() {
thrown.expect(ValidationException.class);
// Not a comprehensive set of invalid delimiters, please check the class's defined pattern.
List someInvalidDelimiters =
new ArrayList<>(Arrays.asList("|", "!", "@", "a", "1", ",", "{", ")"));
for (String invalidDelimiter : someInvalidDelimiters) {
PathTemplate.create(
String.format("projects/{project=*}/zones/{zone_a}%s{zone_b}", invalidDelimiter));
thrown.expectMessage(
String.format(
"parse error: invalid complex resource ID delimiter character in '%s'",
String.format("{zone_a}%s{zone_b}", invalidDelimiter)));
}
}

This test passes even though when the delimiter is {, the exception thrown actually contains the message:
parse error: missing or 2+ consecutive delimiter characters in '{zone_a}{{zone_b}'.

This is because of the way Junit4 handles exceptions and statefulness - it checks for an initial thrown.expect(ValidationException.class) and initial thrown.expectMessage(), which once passed, it considers it passed for the entire test.

We will need to either rewrite this test to account for "{" delimiter, or potentially refactor the way template works such that the exception message matches the initial test.

@alicejli alicejli added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 15, 2024
@blakeli0 blakeli0 added priority: p3 Desirable enhancement or fix. May not be included in next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels May 21, 2024
@blakeli0
Copy link
Collaborator

We should enable this test with all the passing cases. For the failing case, we should either remove it or move it to the above test that has the same error message.

@JoeWang1127 JoeWang1127 self-assigned this May 27, 2024
JoeWang1127 added a commit that referenced this issue May 28, 2024
Fix #2776 and #2778.

The failing tests are in the same file, thus combine into one pull
request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants