Skip to content

Add debugging information for Firestore integration tests #5746

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
BenWhitehead merged 4 commits into from
Jul 23, 2019
Merged

Add debugging information for Firestore integration tests #5746

BenWhitehead merged 4 commits into from
Jul 23, 2019

Conversation

@BenWhitehead
Copy link
Contributor

@BenWhitehead BenWhitehead commented Jul 15, 2019

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

@BenWhitehead BenWhitehead requested review from chingor13 and kolea2 July 15, 2019 21:37
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 15, 2019
@codecov
Copy link

codecov bot commented Jul 16, 2019 •
edited
Loading

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 Files Coverage Δ Complexity Δ
.../java/com/google/cloud/testing/CommandWrapper.java 90.9% <0%> (-6.07%) 13% <0%> (ø)
...in/java/com/google/cloud/storage/StorageBatch.java 88% <0%> (-4%) 13% <0%> (ø)
...m/google/cloud/vision/v1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...gle/cloud/storage/testing/RemoteStorageHelper.java 61.98% <0%> (-3.06%) 9% <0%> (ø)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java 63.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
Contributor Author

BenWhitehead commented Jul 16, 2019

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)

@BenWhitehead
Add debugging information for Firestore integration tests
4d5aa59 
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
Contributor

kolea2 commented Jul 18, 2019

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
Contributor Author

BenWhitehead commented Jul 18, 2019

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
Contributor Author

BenWhitehead commented Jul 18, 2019

@kolea2 Updated message per our offline conversation.

@BenWhitehead
Copy link
Contributor Author

BenWhitehead commented Jul 22, 2019

@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.

chingor13
chingor13 requested changes Jul 22, 2019
View reviewed changes
BenWhitehead and others added 2 commits July 22, 2019 16:44
@BenWhitehead @chingor13
use realpath instead of cd && pwd
498c7af 
Co-Authored-By: Jeff Ching <chingor@google.com>
@BenWhitehead
Update comment
ac67b11
chingor13
chingor13 approved these changes Jul 22, 2019
View reviewed changes
...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java
@Override
public void onEvent(
@Nullable QuerySnapshot value, @Nullable FirestoreException error) {
System.out.printf("onEvent(value : %s, error : %s)%n", value, error);
Copy link
Contributor

@chingor13 chingor13 Jul 22, 2019

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
Contributor Author

@BenWhitehead BenWhitehead Jul 22, 2019

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.

@BenWhitehead BenWhitehead merged commit caac73b into googleapis:master Jul 23, 2019
@BenWhitehead BenWhitehead deleted the firestore-it-hang-debug branch July 23, 2019 15:47
@chingor13 chingor13 mentioned this pull request Jul 24, 2019
Merged
@BenWhitehead BenWhitehead mentioned this pull request Jul 29, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chingor13 chingor13 chingor13 approved these changes

@kolea2 kolea2 Awaiting requested review from kolea2

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BenWhitehead @kolea2 @chingor13 @googlebot