Skip to content

Add debugging information for Firestore integration tests - #5746

Merged
BenWhitehead merged 4 commits into
googleapis:masterfrom
BenWhitehead:firestore-it-hang-debug
Jul 23, 2019
Merged

Add debugging information for Firestore integration tests#5746
BenWhitehead merged 4 commits into
googleapis:masterfrom
BenWhitehead:firestore-it-hang-debug

Conversation

@BenWhitehead

Copy link
Copy Markdown
Contributor

Add an assertion to verify the expected number of events are delivered
to the queryWatch test. Awaiting the semaphore now has a 60sec timeout.
If not satisfied within that timespan the test will fail, and provide
an assertion message of the progress.

Updates to allow running builds locally

  • Update build script to cd to checkout directory relative to itself
    instead of using a hard-coded path.
  • Update build script to be tolerant of absolute paths specified for
    GOOGLE_APPLICATION_CREDENTIALS

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2019
@codecov

codecovBot commented Jul 16, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5746 into master will decrease coverage by 0.97%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #5746 +/- ##
============================================
- Coverage 47.09% 46.12% -0.98% + Complexity 25079 24189 -890 
============================================
Files 2389 2456 +67 Lines 259750 262236 +2486 Branches 29400 29602 +202 ============================================
- Hits 122329 120953 -1376 - Misses 128458 132172 +3714 - Partials 8963 9111 +148
Impacted FilesCoverage ΔComplexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java90.9% <0%> (-6.07%)13% <0%> (ø)
...in/java/com/google/cloud/storage/StorageBatch.java88% <0%> (-4%)13% <0%> (ø)
...m/google/cloud/vision/v1/ImageAnnotatorClient.java58.69% <0%> (-3.31%)15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java58.69% <0%> (-3.31%)15% <0%> (-4%)
...gle/cloud/storage/testing/RemoteStorageHelper.java61.98% <0%> (-3.06%)9% <0%> (ø)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java47.05% <0%> (-2.95%)9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java47.05% <0%> (-2.95%)9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java48.57% <0%> (-2.78%)10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java48.57% <0%> (-2.78%)10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java63.41% <0%> (-2.5%)12% <0%> (-3%)
... and 858 more

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 628637b...ac67b11. Read the comment docs.

@BenWhitehead

Copy link
Copy Markdown
ContributorAuthor

An example of the new diagnostic assertion:

java.lang.AssertionError:
did not receive all expected messages within the deadline.
expectedOperations = [case 0, case 1, case 2, case 3, case 4, case 5]
actualOperations = [case 0]
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.assertTrue(Assert.java:41)
at com.google.cloud.firestore.it.ITSystemTest.queryWatch(ITSystemTest.java:968)

Add an assertion to verify the expected number of events are delivered
to the queryWatch test. Awaiting the semaphore now has a 60sec timeout.
If not satisfied within that timespan the test will fail, and provide
an assertion message of the progress.
Updates to allow running builds locally
* Update build script to cd to checkout directory relative to itself
instead of using a hard-coded path.
* Update build script to be tolerant of absolute paths specified for
GOOGLE_APPLICATION_CREDENTIALS
@kolea2

Copy link
Copy Markdown
Contributor

So does this mean it failed on case 0, or finished case 0 and failed in case 1? I would add clarifying language in the error message to demonstrate this.

@BenWhitehead

Copy link
Copy Markdown
ContributorAuthor

So does this mean it failed on case 0, or finished case 0 and failed in case 1? I would add clarifying language in the error message to demonstrate this.

It's saying that it expected 6 cases to be performed, but only 1 of them was performed. The cases reported in actual are what actually happened. The main thing is that the number of expected async operations didn't happen during the test run. What would you expect as the reported message?

@BenWhitehead

Copy link
Copy Markdown
ContributorAuthor

@kolea2 Updated message per our offline conversation.

@BenWhitehead

Copy link
Copy Markdown
ContributorAuthor

@chingor13 Can you take a look at my changes to .kokoro/build.sh and give it a +1. All of the builds are running in this PR so I think everything should be working well.

Comment thread.kokoro/build.sh Outdated
Comment thread.kokoro/build.sh
BenWhiteheadand others added 2 commits July 22, 2019 16:44
Co-Authored-By: Jeff Ching <chingor@google.com>
@Override
public void onEvent(
@Nullable QuerySnapshot value, @Nullable FirestoreException error) {
System.out.printf("onEvent(value : %s, error : %s)%n", value, error);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: is this intentionally left here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, I'm wanting to see the events flowing in vs the switch branches below being hit. I haven't yet ruled out if there is some sort of race happening on the events being delivered.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@BenWhitehead@kolea2@chingor13@googlebot