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

fix: skip some tests when run against the emulator #933

Merged
merged 16 commits into from
May 12, 2020

Conversation

skuruppu
Copy link
Contributor

@skuruppu skuruppu commented May 1, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #932 🦕

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 1, 2020
@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #933 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #933   +/-   ##
=======================================
  Coverage   98.20%   98.20%           
=======================================
  Files          21       21           
  Lines       19959    19959           
  Branches     1069     1069           
=======================================
  Hits        19601    19601           
  Misses        354      354           
  Partials        4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f2d52d...b07cc61. Read the comment docs.

@hengfengli
Copy link
Contributor

Hi @AVaksman, @bcoe, do you have any idea why ci/windows fails? I can see it also fails in another PR for the same reason: #931

@hengfengli
Copy link
Contributor

@skuruppu I have skipped all needed tests and fixed related errors. I am not sure:

  1. why "ci / windows" fails. It happens in other PRs as well.
  2. Who should I assign this as a reviewer? I guess you're the PR creator and I can't assign it to you.

@@ -1051,13 +1070,23 @@ describe('Spanner', () => {
});

after(async () => {
await restoreDatabase.delete();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would doing this here work:?

if (emulatorEnabled) {
  this.skip();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work. I tried this at the beginning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I got the following error:

     Error: `this.skip` forbidden

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 did some research. The documentation says to just do a return instead of a skip. I updated the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. Thanks for making this more elegant.

@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 5, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 5, 2020
@hengfengli hengfengli added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 5, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 6, 2020
@skuruppu skuruppu requested a review from olavloite May 6, 2020 04:49
@hengfengli
Copy link
Contributor

@skuruppu I think we need to clean up the leftover instances in the GCP project. It fails to add a new instance.

@skuruppu
Copy link
Contributor Author

skuruppu commented May 7, 2020

I agree, it's weird though because the tests actually try to clean up the instances before it creates a new one. I'm not sure why that's not working. I suppose it's because we try to create an instance before deleting the old ones?

@hengfengli
Copy link
Contributor

I guess the instance ID is generated by date:

const INSTANCE_ID = process.env.SPANNERTEST_INSTANCE || `test-instance-${date}`;

The old one from yesterday is not deleted.

@skuruppu
Copy link
Contributor Author

skuruppu commented May 7, 2020

I created #955, let's see if it runs.

@hengfengli hengfengli added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 7, 2020
@skuruppu skuruppu requested a review from stephenplusplus May 8, 2020 05:10
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 8, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 8, 2020
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@skuruppu skuruppu added automerge Merge the pull request once unit tests and other checks pass. and removed automerge Merge the pull request once unit tests and other checks pass. labels May 12, 2020
@skuruppu skuruppu merged commit 2d91757 into googleapis:master May 12, 2020
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.

Skip some system tests when run against the emulator
6 participants