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

tests: add system tests to check error details #1076

Merged
merged 21 commits into from Nov 10, 2021
Merged

tests: add system tests to check error details #1076

merged 21 commits into from Nov 10, 2021

Conversation

atulep
Copy link
Contributor

@atulep atulep commented Nov 8, 2021

System tests to check error details added in googleapis/python-api-core#286.

@atulep atulep requested a review from a team November 8, 2021 17:52
@atulep atulep requested a review from a team as a code owner November 8, 2021 17:52
@atulep atulep changed the title Adds system tests to check error details. feat: Adds system tests to check error details. Nov 8, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2021
@atulep
Copy link
Contributor Author

atulep commented Nov 8, 2021

The commit history in this PR is misleading. It contains some of my older commits unrelated to this PR. Any ideas how to fix it?

@tseaver tseaver changed the title feat: Adds system tests to check error details. tests: add system tests to check error details Nov 8, 2021
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

In addition to my comments inline, two things:

  • These are unit tests, not system tests (they don't exercise actual over-the-wire API calls).
  • They belong in python-api-core, not in this repository.

tests/system/test_error_details.py Outdated Show resolved Hide resolved
tests/system/test_error_details.py Show resolved Hide resolved
@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 8, 2021
@tseaver
Copy link
Contributor

tseaver commented Nov 8, 2021

@atulep For future reference, you can rebase your branch against main (e.g., git rebase main) to clean up the previous history.

@atulep
Copy link
Contributor Author

atulep commented Nov 8, 2021

In addition to my comments inline, two things:

  • These are unit tests, not system tests (they don't exercise actual over-the-wire API calls).
  • They belong in python-api-core, not in this repository.

@tseaver
Tests send API requests to showcase server and assert errors returned from real server response.

We want gapic clients to have access to error.details, so we should test it here. Additionally,python-api-core doesn't have any existing infrastructure for system tests, which suggested to me it's not a suitable place.

@tseaver
Copy link
Contributor

tseaver commented Nov 8, 2021

@atulep I apologize -- I somehow missed seeing that these tests use the echo fixture.

Please do rebase your branch against the current main and force push, e.g.:

$ git fetch --all --prune
$ git checkout main
$ git checkout error_details
$ git rebase main
$ git push -f origin error_details

I would also avoid re-using a branch after merging its PR.

@atulep
Copy link
Contributor Author

atulep commented Nov 9, 2021

@tseaver, I followed your script, but it gave me:

$ git rebase master
Current branch error_details is up to date.

Maybe it's easier to start a fresh PR with correct history.

Copy link

@summer-ji-eng summer-ji-eng left a comment

Choose a reason for hiding this comment

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

It's not blocker, but please add a test case for error_info once the feature merged.

Thank you very much. :)

@atulep atulep removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 10, 2021
@atulep atulep merged commit a03bc22 into googleapis:master Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants