fix: deflake-ify ITSystemTest#queryWatch #107

BenWhitehead · 2020-02-13T23:22:29Z

queryWatch has been flaky because several things are happening in the
single event stream. This fix splits up the different scenarios being
tested into their own respective tests and updates the assertions to be
more specific to the scenario as well as handling the semantics of the
backend better.

queryWatch has been flaky because several things are happening in the single event stream. This fix splits up the different scenarios being tested into their own respective tests and updates the assertions to be more specific to the scenario as well as handling the semantics of the backend better.

codecov · 2020-02-13T23:48:20Z

Codecov Report

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

@@             Coverage Diff              @@
##             master     #107      +/-   ##
============================================
- Coverage     71.97%   71.56%   -0.41%     
+ Complexity      997      968      -29     
============================================
  Files            62       62              
  Lines          5323     5202     -121     
  Branches        599      579      -20     
============================================
- Hits           3831     3723     -108     
+ Misses         1304     1299       -5     
+ Partials        188      180       -8

Impacted Files	Coverage Δ	Complexity Δ
...cloud/firestore/collection/ImmutableSortedMap.java	`13.95% <0%> (-1.96%)`	`1% <0%> (ø)`
...oogle/cloud/firestore/v1beta1/FirestoreClient.java	`54.63% <0%> (-1.56%)`	`25% <0%> (-6%)`
.../java/com/google/cloud/firestore/Precondition.java	`68.18% <0%> (-1.39%)`	`12% <0%> (-3%)`
...com/google/cloud/firestore/v1/FirestoreClient.java	`76.28% <0%> (-0.86%)`	`31% <0%> (-6%)`
...loud/firestore/v1beta1/stub/GrpcFirestoreStub.java	`93.67% <0%> (-0.57%)`	`16% <0%> (-1%)`
...a/com/google/cloud/firestore/FirestoreOptions.java	`36.44% <0%> (-0.54%)`	`7% <0%> (ø)`
...loud/firestore/v1/stub/GrpcFirestoreAdminStub.java	`94.32% <0%> (-0.46%)`	`15% <0%> (-1%)`
...ain/java/com/google/cloud/firestore/FieldMask.java	`87.09% <0%> (-0.41%)`	`13% <0%> (-2%)`
.../firestore/v1beta1/stub/FirestoreStubSettings.java	`79.26% <0%> (-0.38%)`	`22% <0%> (-1%)`
...gle/cloud/firestore/v1/stub/GrpcFirestoreStub.java	`96.04% <0%> (-0.36%)`	`19% <0%> (-1%)`
... and 11 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 fcdbab3...7336a03. Read the comment docs.

kolea2 · 2020-02-18T16:46:23Z

LGTM

BenWhitehead · 2020-02-25T22:23:13Z

@schmidt-sebastian Can you get a quick review from you on this PR?

schmidt-sebastian

Thanks, this is much better than writing Perf (and pretty good in general). I think we could simplify the test a bit by relying on more deterministic ordering.

schmidt-sebastian · 2020-02-25T22:24:38Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  }
+
+  /*
+  Start a listener on a query with an empty result set


Please fix (here and everywhere).

What fix would you like me to apply here? Turn this into a sentence?

Sorry, I should have been more clear (or rather just "be clear"). Can you turn this into a JSDoc comment and prefix all lines with *?

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

schmidt-sebastian · 2020-02-25T22:26:16Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  that matches the query should result in an ADDED event
+   */
+  @Test
+  public void emptyResults_newDocument_ADDED()


This name could use a sprinkle of the Java Style Guide :)

This type of scenario is specifically called out in the style guide: http://go/java-style#s5.2.3-method-names

Underscores may appear in JUnit test method names to separate logical components of the name, with each component written in lowerCamelCase

Is your ask here for me to not use the upper enum names? i.e. s/ADDED/added/?

Interesting. I was going to suggest to use camel case throughout, but I withdraw my suggestion hereby.

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

schmidt-sebastian · 2020-02-25T22:27:49Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+
+    ListenerAssertions la = listener.assertions();
+    la.noError();
+    la.eventCountIsAnyOf(Range.closed(1, 2));


If you wait for the initial empty event, the number of events is deterministic.

Will update

schmidt-sebastian · 2020-02-25T22:29:02Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  match the query should result in an ADDED event
+   */
+  @Test
+  public void emptyResults_modifiedDocument_ADDED()


Same comment regarding the style guide.

schmidt-sebastian · 2020-02-25T22:29:57Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+    List<ListenerEvent> receivedEvents = listener.receivedEvents;
+    ListenerRegistration registration = query.addSnapshotListener(listener);
+
+    try {


This test would be easier to follow if you waited for the initial snapshot. Do you mind updating all tests?

I'll make sure all tests wait for the initial event

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

schmidt-sebastian · 2020-02-25T22:34:08Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+      cdls = new EnumMap<>(DocumentChange.Type.class);
+      cdls.put(DocumentChange.Type.ADDED, new CountDownLatch(addedInitial));
+      cdls.put(DocumentChange.Type.MODIFIED, new CountDownLatch(modifiedInitial));
+      cdls.put(DocumentChange.Type.REMOVED, new CountDownLatch(removedInitial));


The way the Watch is used in this test (adding or modifying a document at a time) is actually deterministic. By splitting these updates across types, you add some complexity to the tests that I don't think you need.

I tried removing the individual CountDownLatches here and ran into several tests becoming more flaky, so I'd like to leave this for now and spend more time digging into it later.

…on to facilitate diagnostics

BenWhitehead · 2020-02-26T23:37:03Z

@schmidt-sebastian I've pushed several commits that have addressed most of your comments. (I added some more debugging information to the assertions in the case that they fail to hopefully aid in diagnostics when we see failures. I've seen a few failures working on things, seemingly related to events taking longer than we allow the await to block. Is there a defined SLO for events being delivered?)

schmidt-sebastian

@schmidt-sebastian I've pushed several commits that have addressed most of your comments. (I added some more debugging information to the assertions in the case that they fail to hopefully aid in diagnostics when we see failures. I've seen a few failures working on things, seemingly related to events taking longer than we allow the await to block. Is there a defined SLO for events being delivered?)

I was only able to find this internally: https://g3doc.corp.google.com/cloud/datastore/g3doc/SLO.md?cl=head

schmidt-sebastian · 2020-02-26T23:14:40Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  }
+
+  /*
+  Start a listener on a query with an empty result set


Sorry, I should have been more clear (or rather just "be clear"). Can you turn this into a JSDoc comment and prefix all lines with *?

schmidt-sebastian · 2020-02-26T23:15:56Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  that matches the query should result in an ADDED event
+   */
+  @Test
+  public void emptyResults_newDocument_ADDED()


Interesting. I was going to suggest to use camel case throughout, but I withdraw my suggestion hereby.

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

schmidt-sebastian · 2020-02-29T04:19:30Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+    ListenerRegistration registration = query.addSnapshotListener(listener);
+
+    try {
+      randomColl.document("doc").set(map("foo", "bar")).get(5, TimeUnit.SECONDS);


Sounds good.

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

…ed arguments

…pected for that type of event

BenWhitehead · 2020-03-02T18:10:26Z

@schmidt-sebastian I've taken care of all the nits, and I update the await conditions to be a multiple of the number of events rather than the firm 5 seconds. This will at least scale our tolerance in the test since.

schmidt-sebastian

Thanks for all the updates. Looking forward to non-flaky development on Firestore :)

schmidt-sebastian · 2020-03-03T18:18:59Z

google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryWatchTest.java

+  /**
+   *
+   *
+   * <ol>


Nit: Are the two empty lines above from code formatting?

They are indeed. I wasn't able to figure out a way to remove them.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2020

BenWhitehead added 2 commits February 13, 2020 18:42

chore: copyright header

4086977

chore: run formatter

a0aa026

BenWhitehead requested a review from kolea2 February 14, 2020 17:57

schmidt-sebastian suggested changes Feb 25, 2020

View reviewed changes

BenWhitehead added 6 commits February 26, 2020 15:07

cleanup: clarify test scenario comments

bf14aa9

cleanup: s/e/event/g

324ed4f

cleanup: ensure all tests wait for listener activation

03a8b69

cleanup: update all tests to use helper assertions

02a2ee0

cleanup: update assertion failures to include summary state informati…

e93b231

…on to facilitate diagnostics

cleanup: rename EventsCDL -> EventsCountDownLatch

dbb20d3

cleanup: run formatter

23c9945

schmidt-sebastian reviewed Feb 29, 2020

View reviewed changes

schmidt-sebastian approved these changes Feb 29, 2020

View reviewed changes

BenWhitehead added 5 commits March 2, 2020 12:38

cleanup: expand acronyms

7c42924

cleanup: remove duplicate events in ListenerAssertions

f699c7a

cleanup: javadoc-ify test scenario comments

2a0fee6

cleanup: create builder for QuerySnapshotEventListener to provide nam…

7f34c9d

…ed arguments

cleanup: update await time to be a multiple of how many events are ex…

7336a03

…pected for that type of event

schmidt-sebastian approved these changes Mar 3, 2020

View reviewed changes

kolea2 approved these changes Mar 3, 2020

View reviewed changes

BenWhitehead merged commit f701c67 into googleapis:master Mar 3, 2020

BenWhitehead deleted the deflake-queryWatch branch March 3, 2020 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: deflake-ify ITSystemTest#queryWatch #107

fix: deflake-ify ITSystemTest#queryWatch #107

BenWhitehead commented Feb 13, 2020

codecov bot commented Feb 13, 2020 •

edited

kolea2 commented Feb 18, 2020

BenWhitehead commented Feb 25, 2020

schmidt-sebastian left a comment

schmidt-sebastian Feb 25, 2020

BenWhitehead Feb 26, 2020

schmidt-sebastian Feb 26, 2020

schmidt-sebastian Feb 25, 2020

BenWhitehead Feb 26, 2020

schmidt-sebastian Feb 26, 2020

schmidt-sebastian Feb 25, 2020

BenWhitehead Feb 26, 2020

schmidt-sebastian Feb 25, 2020

schmidt-sebastian Feb 25, 2020

BenWhitehead Feb 26, 2020

schmidt-sebastian Feb 25, 2020

BenWhitehead Feb 26, 2020

BenWhitehead commented Feb 26, 2020

schmidt-sebastian left a comment

schmidt-sebastian Feb 26, 2020

schmidt-sebastian Feb 26, 2020

schmidt-sebastian Feb 29, 2020

BenWhitehead commented Mar 2, 2020

schmidt-sebastian left a comment

schmidt-sebastian Mar 3, 2020

BenWhitehead Mar 3, 2020

fix: deflake-ify ITSystemTest#queryWatch #107

fix: deflake-ify ITSystemTest#queryWatch #107

Conversation

BenWhitehead commented Feb 13, 2020

codecov bot commented Feb 13, 2020 • edited

Codecov Report

kolea2 commented Feb 18, 2020

BenWhitehead commented Feb 25, 2020

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWhitehead commented Feb 26, 2020

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWhitehead commented Mar 2, 2020

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2020 •

edited