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

test: add tests for update with exists=false precondition #84

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

dconeybe
Copy link
Contributor

This is a follow-up to #82 which updated the test for the exists precondition.

@dconeybe dconeybe requested a review from a team as a code owner January 20, 2024 02:29
@dconeybe
Copy link
Contributor Author

@jskeet FYI

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Looks fine to me (albeit a little odd as this is effectively "create" isn't it?). Testing it against the current .NET implementation would be slightly fiddly, but I expect it to be fine.

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Undoing previous LGTM - it's not clear whether we do want to prohibit this. (We've never prohibited it in .NET - are we sure this won't break people? What does the server do?)

},
"jsonData": "{\"a\": 1}",
"isError": true,
"request": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted in chat, let's remove this given that it's meant to be an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Much clearer, thanks. And I've verified that it won't break the public API surface for .NET as we don't allow public specification of that precondition anywhere. (I'll need to add validation into the method in order to get the conformance tests to pass, but that's okay.)

@dconeybe
Copy link
Contributor Author

@ddelgrosso1 Are you able and willing to give approval for this PR too? It's a follow-up to #82. Feel free to DM me and/or @jskeet if you would like any clarifications. Thank you.

@jskeet jskeet merged commit 76305a9 into googleapis:main Jan 23, 2024
3 of 4 checks passed
@dconeybe dconeybe deleted the UpdateExistsTrueAndFalseTests branch January 23, 2024 13:35
dconeybe added a commit to dconeybe/java-conformance-tests that referenced this pull request Jan 23, 2024
googleapis/conformance-tests@d160e3a chore: update go version to 1.21 ([googleapis#86](googleapis/conformance-tests#86))
googleapis/conformance-tests@76305a9 test: add tests for update with exists=false precondition ([googleapis#84](googleapis/conformance-tests#84))
googleapis/conformance-tests@3de4678 tests: Reverse position on explicit "must exist" for update
googleapis/conformance-tests@3264aa6 chore: Reorder query parameters in expected URL for signature test

Full diff: googleapis/conformance-tests@f5b5d0f...d160e3a
cojenco pushed a commit to googleapis/java-conformance-tests that referenced this pull request Jan 23, 2024
…f19 (#643)

googleapis/conformance-tests@d160e3a chore: update go version to 1.21 ([#86](googleapis/conformance-tests#86))
googleapis/conformance-tests@76305a9 test: add tests for update with exists=false precondition ([#84](googleapis/conformance-tests#84))
googleapis/conformance-tests@3de4678 tests: Reverse position on explicit "must exist" for update
googleapis/conformance-tests@3264aa6 chore: Reorder query parameters in expected URL for signature test

Full diff: googleapis/conformance-tests@f5b5d0f...d160e3a
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.

None yet

2 participants