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.

Sign up for GitHub

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

Jump to bottom

Add spanner system tests: #4371

Merged
merged 2 commits into from
Nov 10, 2017
Merged

Add spanner system tests: #4371

merged 2 commits into from
Nov 10, 2017

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 9, 2017

  • Bind timestamp.
  • Bind string array.
  • Bind empty string array.
  • Bind string to null.
  • Query returning array of -Inf, +Inf, NaN.

Toward #4364.

@tseaver tseaver added api: spanner Issues related to the Spanner API. testing labels Nov 9, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2017
@tseaver tseaver mentioned this pull request Nov 9, 2017
Closed
19 tasks
dhermes
dhermes approved these changes Nov 9, 2017
View reviewed changes
Copy link
Contributor

@dhermes dhermes left a comment

Choose a reason for hiding this comment

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

LGTM, though that test case is getting pretty long. You should consider breaking it up.

@tseaver
Copy link
Contributor Author

tseaver commented Nov 9, 2017

@dhermes I split the "transfinite" cases out into a separate method. Code is otherwise unchanged: I will merge when Circle is green.

@tseaver
Add spanner system tests:
57fd9d8 
- Bind timestamp.
- Bind string array.
- Bind empty string array.
- Bind string to null.
- Query returning array of -Inf, +Inf, NaN.
@tseaver
Split up long testcase method.
03304c9 
Group all -inf / +inf / nan cases together.
@tseaver tseaver force-pushed the 4364-spanner-systest-bind-query branch from ce26b03 to 03304c9 Compare November 9, 2017 21:51
@tseaver
Copy link
Contributor Author

tseaver commented Nov 9, 2017

Rebased to pick up hardening from merged #4374.

chemelnucfin
chemelnucfin approved these changes Nov 9, 2017
View reviewed changes
Copy link
Contributor

@chemelnucfin chemelnucfin left a comment

Choose a reason for hiding this comment

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

LGTM. But agree with @dhermes comment about too many checks in one method. No big deal though.

Also, may I suggest to use origin (your own) for branching instead of upstream (gcp) to keep branches tidy?

@dhermes
Copy link
Contributor

dhermes commented Nov 9, 2017

Also, may I suggest to use origin (your own) for branching instead of upstream (gcp) to keep branches tidy?

This was intentional by @tseaver. When we push to the upstream remote, the system tests get run (they get skipped on PR builds otherwise).

But good advice otherwise!

@chemelnucfin
Copy link
Contributor

@dhermes wait, so if I make a PR the system tests don't run on CI?

@dhermes
Copy link
Contributor

dhermes commented Nov 9, 2017

@chemelnucfin That's correct. It's a security concern, since anyone can send a PR. Check out one of your builds, it'll say "skipped" by the system tests.

@tseaver tseaver merged commit 2bf694b into master Nov 10, 2017
@tseaver tseaver deleted the 4364-spanner-systest-bind-query branch November 10, 2017 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chemelnucfin chemelnucfin chemelnucfin approved these changes

@dhermes dhermes dhermes approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tseaver @dhermes @chemelnucfin @googlebot