Firestore: conformance tests hiding failures

[tseaver]

Since dd4b646 (PR #4851), the test_cross_language testcase has been swallowing errors.

And therefore we have a bunch of bugs to fix. :(

[tseaver]

/cc @schmidt-sebastian

[schmidt-sebastian]

The cross platform test cases - especially those that only verify that an error has thrown - are not a replacement for unit tests. There are a very helpful tool when it comes to ensuring API conformance, but even without these tests, we should aim for high test coverage.

[tseaver]

@schmidt-sebastian: we do have good test coverage, but the failures masked by this bug are cases where our tests are asserting wrong behavior.

Edit: I just checked, and we have 100% unit test coverage (including branches) even with the conformance tests not running.

[mcdonc]

At least some subset of the failures are related to the decision to not pass "options" to document.set, instead electing to pass a merge boolean, which was an API change I guess. This is a problem because some set tests pass options in cases where merge is true and those options get ignored.

This is the commit that introduced the API change:

421efcd

[mcdonc]

Here's sample failure output from the conformance tests demonstrating this (via set-17.testproto). Note that the expected "update mask" and payload is only "a" but the actual update mask and payload contain both "a" and "b". This is because set-17.testproto contains a "fields: a" option that gets ignored.

E   AssertionError: Expected call: commit('projects/projectID/databases/(default)', [update {
E     name: "projects/projectID/databases/(default)/documents/C/d"
E     fields {
E       key: "a"
E       value {
E         integer_value: 1
E       }
E     }
E   }
E   update_mask {
E     field_paths: "a"
E   }
E   ], metadata=[('google-cloud-resource-prefix', 'projects/projectID/databases/(default)')], transaction=None)
E   Actual call: commit('projects/projectID/databases/(default)', [update {
E     name: "projects/projectID/databases/(default)/documents/C/d"
E     fields {
E       key: "a"
E       value {
E         integer_value: 1
E       }
E     }
E     fields {
E       key: "b"
E       value {
E         integer_value: 2
E       }
E     }
E   }
E   update_mask {
E     field_paths: "a"
E     field_paths: "b"
E   }
E   ], metadata=[('google-cloud-resource-prefix', 'projects/projectID/databases/(default)')], transaction=None)

[tseaver]

See some discussion of the MergeOption bit in #4111.

[mcdonc]

Good find. It looks like the decision to use a merge= boolean instead of a MergeOption was cosmetic.

#4111 (comment)

I suspect we should just revert 421efcd but I'm not sure how an API change like this should be shepherded.

[mcdonc]

It's not quite as simple as reverting the effect of that commit, unfortunately. I think I understand now why the commit was applied.

Various methods of DocumentReference accept an "option" argument. An option is a "write option", which is an instance of a class that inherits from a common base class. These are supplied only on write ops like set and update. The state of the code prior to the commit attempted to characterize a merge as an option, but only one type of option may be supplied to any of the methods (there are other types of options such as an exists option, and a last_update option). This wasn't a problem in the past, because there were only two options previously (exists and last_update) and they were mutually exclusive. However, with the introduction of merge as an option, I think it broke the "all options are mutually exclusive from one another" assumption that had been in effect until then. This is evidenced by e.g. https://github.com/googleapis/google-cloud-python/blob/master/firestore/google/cloud/firestore_v1beta1/_helpers.py#L930 where it was changed to accept two arguments, exists and merge, which had each once been options; that code duplicates for "exists" what the client.ExistsOption.modify_write method is doing, doing an end run around the options system entirely.

So I think we either need to:

• Allow DocumentReference.set/update/etc and its callees to accept multiple options (e.g. add back option= paramter to those methods and allow option to be a list and do duck typing on it further down the stack). We can do a sanity check before we apply the options that they aren't mutually exclusive.

• Do away with the concept of "options" entirely; instead, hardcode the updates to the protobuf that are implied by options based on flags like the referenced commit did for the combination of exists and merge.

I am partial to the first option.

[schmidt-sebastian]

Please follow the APIs as outlined in the specification and as implemented by the other SDKs.

Set - the Protobuf contains only Write.Document
Set with merge:true - the Protobuf contains Write.Document and Write.UpdateMask, where the update mask contains the field paths of all leaf nodes.
Set with mergePaths - the Protobuf contains Write.Document and Write.UpdateMask, where the update mask contains the field paths as defined in mergePaths.
Update - the Protobuf contains Write.Document (with fields split on dots), Write.UpdateMask with the field paths specified in the input, and Precondition.exists
Update with Precondition lastUpdateTime - the Protobuf contains Write.Document (with fields split on dots), Write.UpdateMaskwith the field paths specified in the input, and Precondition.lastUpdateTime

Please take a look at the reference implementation in Node or in Java if there are any concerns.

[mcdonc]

For the record, and in case anyone but me works on this:

I assumed the set-XX.textproto testcases in tests/unit/testdata had been passing before the #4851 MergeOption PR was merged, but I was wrong, e.g. https://github.com/chemelnucfin/google-cloud-python/blob/6a195488e188b643696a665ee1a63acbadf75d46/firestore/tests/unit/test_cross_language.py#L65 avoids actually testing these cases.

This means means that it wasn't just the MergeOption commit which broke "set" tests (however, never reporting the breakage). It turns out that the set-XX.textproto tests weren't actually being run even before that, so we never actually verified conformance for set, ever.

[chemelnucfin]

@mcdonc Nice to meet you.

As you mentioned here, I decided to design to pass "merge=True" to try to be similar to the node implementation here and here

Digging through my commits, this does pass all 83 conformance tests at the time.

However, I either must have squashed the wrong branch onto this or as I was implementing the MergeOption, I must have inadvertently broke tests.

For the firestore team, especially @schmidt-sebastian @jba I am deeply sorry about any inconvenience I may have caused and I regretfully ended this badly in my last week. However, it was an honest mistake and I did not intend to claim that I passed the conformance test when I had not.

I am willing to fix this if I can coordinate with @mcdonc and @tseaver.

[chemelnucfin]

@mcdonc @tseaver To whomever is working on this, I am fairly certain I had squashed the wrong branch because the structure here from after passing the conformance tests at the time is different than what I had ended up with in the master branch.

My current branch is failing 4 of the tests at the time, but it is probably the direction I had intended to go as I tried to do away with the option. It does seem like I need more info than a boolean in merge in pbs_for_set such as a list of paths though, so I wasn't sure what I was thinking back then.

I am by no means finished, and I will look at it more when I could, but feel free to reach out to me if I can be of help.