Skip to content

test: add back emulator test workflow #71

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

Merged
merged 16 commits into from
Jun 17, 2021
Merged

test: add back emulator test workflow #71

merged 16 commits into from
Jun 17, 2021

Conversation

skuruppu
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 20, 2021
@skuruppu skuruppu requested a review from IlyaFaer May 20, 2021 05:04
@skuruppu
Copy link
Contributor Author

@IlyaFaer would you be able to take this PR and skip the relevant tests to get the workflow green?

I tried to see if I can debug it but I'm not sure where the primary key test is failing :(

@IlyaFaer
Copy link
Contributor

@skuruppu, yeah, I'm taking a look.

@google-cla
Copy link

google-cla bot commented May 20, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels May 20, 2021
@IlyaFaer
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels May 20, 2021
@IlyaFaer
Copy link
Contributor

IlyaFaer commented May 20, 2021

I'm not sure where the primary key test is failing

Hm-m... Does Spanner emulator support tables with an empty primary key? The error ZETASQL_RET_CHECK failure (backend/schema/validators/table_validator.cc:193) !table->primary_key_.empty() looks pretty much like the one caused by a lack of primary key columns definition. There are a lot of tests, which are using tables with empty primary keys - as they are inserting only single one row, they are okay to use such primary keys.

Yeah, that's probably the thing that forced us to use only live service in testing (I don't remember clearly, but the error is definitely looks familiar) - it'll require a long list of overrides, as many tests using these empty primary keys.

@skuruppu
Copy link
Contributor Author

I ran some tests (using the Node.js client) against the emulator and I was able to successfully create a table with no primary key, insert a row and query it back. So I don't think that's it.

Comment on lines 1012 to 1015
@pytest.mark.skipif(
os.getenv("SPANNER_EMULATOR_HOST"),
reason="Numeric type isn't supported by emulator yet",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think these should fail. The latest release of the emulator does support NUMERIC. See googleapis/nodejs-spanner#1358 where we enabled the tests on Node.js.

@IlyaFaer
Copy link
Contributor

Returning to this one, taking a closer look...

@IlyaFaer
Copy link
Contributor

IlyaFaer commented May 27, 2021

I ran some tests (using the Node.js client) against the emulator and I was able to successfully create a table with no primary key, insert a row and query it back. So I don't think that's it.

Unfortunately, I'm about to disagree. The first test case failing bad with !table->primary_key_.empty() is ComponentReflectionTest. I've taken a look at its setups, and it appeared it uses two tables without primary keys. I've tried to override the setup method to explicitly set primary keys for those tables, and now ComponentReflectionTest passing fine.

An SQLs without primary key explicitly defined look like this in SQLAlchemy:

CREATE TABLE Singers (
            SingerId     INT64 NOT NULL,
            FirstName    STRING(1024),
            LastName     STRING(1024),
            SingerInfo   BYTES(MAX)
        ) PRIMARY KEY ()

Thus, primary key definition is empty. And seems like it's confusing the emulator.

@google-cla
Copy link

google-cla bot commented Jun 10, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 10, 2021
@AVaksman
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 10, 2021
@google-cla
Copy link

google-cla bot commented Jun 14, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 14, 2021
@HemangChothani
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 14, 2021
Copy link
Contributor

@larkee larkee left a comment

Choose a reason for hiding this comment

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

LGTM

@IlyaFaer
Copy link
Contributor

Several tests are failing with 400 Unable to infer type for parameter a1. The error is going to be fixed in a separate PR for DB API: googleapis/python-spanner#200
Everything else seems to be working fine. Merging.

@IlyaFaer IlyaFaer merged commit 0841046 into main Jun 17, 2021
@IlyaFaer IlyaFaer deleted the add-github-actions branch June 17, 2021 09:13
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.

5 participants