Skip to content

Add systests for read/query w/ concurrent updates. #3632

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
Merged
tseaver merged 5 commits into from
Jul 20, 2017
Merged

Add systests for read/query w/ concurrent updates. #3632

tseaver merged 5 commits into from
Jul 20, 2017

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 19, 2017

No description provided.

@tseaver tseaver added api: spanner Issues related to the Spanner API. testing labels Jul 19, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2017
dhermes
dhermes reviewed Jul 19, 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 pending green CI (should you push a branch to the GCP remote so we can see the system tests?)

spanner/tests/system/test_system.py Outdated
self._check_row_data(rows)

def _transaction_concurrency_helper(self, unit_of_work, pkey):
import threading

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

spanner/tests/system/test_system.py
[[pkey, INITIAL_VALUE]])

# We don't want to run the threads' transactions in the current
# session, which would fail.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

spanner/tests/system/test_system.py Outdated
self.assertEqual(len(rows), 1)
pkey, value = rows[0]
transaction.update(
self.COUNTERS_TABLE,

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor Author

tseaver commented Jul 19, 2017

Test failures look unrelated to our code :<

@theacodes
Copy link
Contributor

theacodes commented Jul 19, 2017

@tseaver I'm working on it, it's a problem in our base image.

dhermes
dhermes reviewed Jul 19, 2017
View reviewed changes
spanner/tests/system/test_system.py Outdated
retry = RetryInstanceState(_has_all_ddl)
retry(self._db.reload)()

class TestException(Exception):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes
dhermes reviewed Jul 19, 2017
View reviewed changes
spanner/tests/system/test_system.py

with session.batch() as batch:
batch.delete(self.TABLE, self.ALL)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@theacodes
Copy link
Contributor

theacodes commented Jul 19, 2017

@tseaver I'm working on it, it's a problem in our base image.

Base image fixed, any test failures are no longer my fault. :)

dhermes
dhermes reviewed Jul 19, 2017
View reviewed changes
spanner/tests/system/test_system.py Outdated
rows = list(transaction.read(self.TABLE, self.COLUMNS, self.ALL))
self.assertEqual(len(rows), 0)
transaction.insert(self.TABLE, self.COLUMNS, self.ROW_DATA)
raise TestException()

This comment was marked as spam.

Sign in to view

@dhermes
Copy link
Contributor

dhermes commented Jul 19, 2017

@tseaver I presume https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/2453 is the build in the GCP remote?

@dhermes
Copy link
Contributor

dhermes commented Jul 19, 2017

@tseaver Rename your custom exception? pytest is trying to collect it:

tests/system/test_system.py::TestException
  cannot collect test class 'TestException' because it has a __init__ constructor

-- Docs: http://doc.pytest.org/en/latest/warnings.html

@tseaver
Copy link
Contributor Author

tseaver commented Jul 19, 2017

Ugh, failures are now due to BQ system test flakiness, Why are we running tests for the other APIs again?

@dhermes
Copy link
Contributor

dhermes commented Jul 19, 2017

Why are we running tests for the other APIs again?

Because no on (you or I included) has written tooling to make the build aware of what it should be building. The relevant code is here https://github.com/GoogleCloudPlatform/google-cloud-python/blob/3d9461b91963fcc6e6a864f6f0eacad3d92bbf2d/test_utils/scripts/get_target_packages.py but IMO it's not worth it since we're breaking up the monorepo soon.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 19, 2017

Breaking up the monorepo doesn't seem to be high up on anybody's priority list ATM.

@tseaver tseaver merged commit f8758df into googleapis:master Jul 20, 2017
@tseaver tseaver deleted the spanner-systests-read-query-concurrent-update branch July 20, 2017 22:18
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
@tseaver @landrito
Add systests for read/query w/ concurrent updates. (googleapis#3632)
16e6ef3 
* Also add systest for user exception aborting transaction.
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
@tseaver @landrito
Add systests for read/query w/ concurrent updates. (googleapis#3632)
d5e958c 
* Also add systest for user exception aborting transaction.
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
@tseaver @landrito
Add systests for read/query w/ concurrent updates. (googleapis#3632)
521de98 
* Also add systest for user exception aborting transaction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

1 more reviewer

@dhermes dhermes dhermes left review comments

Reviewers whose approvals may not affect merge requirements

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 @theacodes @dhermes @googlebot