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

testing: remove nextQuery assertion #377

Merged
merged 1 commit into from
Mar 11, 2019
Merged

testing: remove nextQuery assertion #377

merged 1 commit into from
Mar 11, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Mar 11, 2019

Fixes #375

Since we refactored our strategy for dealing with lingering resources, sometimes nextQuery will be populated with a page of resources from a different test run.

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 11, 2019
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

looks good to me 👍 is there a tracking issue to try to reset state between tests runs, this can definitely start to make for hard to debug test suites.

@@ -176,7 +176,6 @@ describe('BigQuery', () => {
filter: `labels.${GCLOUD_TESTS_PREFIX}`,
});
assert(datasets[0] instanceof Dataset);
assert.strictEqual(nextQuery, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems reasonable to me; in a perfect world, I might try to have a beforeEach that ensures that there's nothing populated in nextQuery before running the test. Is there a tracking issue for this bug? (state between tests can really start to be a pain).

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 there is, @JustinBeckwith might know though?

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files           4        4           
  Lines         544      544           
  Branches       75       75           
=======================================
  Hits          541      541           
  Misses          2        2           
  Partials        1        1

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 e4b0b12...0d8b7ed. Read the comment docs.

@callmehiphop callmehiphop merged commit f70cb9f into googleapis:master Mar 11, 2019
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.

4 participants