[SAT] - Test that STATE emitted during run matches provided sample states

[evantahler]

From https://github.com/airbytehq/oncall/issues/1317 which was solved by #21864

We don't test that syncs /actually emit/ a state message. We should do that.
We should also confirm that the format (e.g. the keys in the state JSON) match the state messages which are provided as inputs to SAT.

To discuss:

• do all or only some properties of a state message need to match?
• can we determine based on the catalog (and assuming per-stream-state) which state messages we expect?

[evantahler]

cc @brianjlai, who has ideas on this topic

[evantahler]

@bechurch this will be story #2, and introduce you to SAT - how we test all connectors, regardless of the language they are programmed in.

[brianjlai]

Just to document the full context of the ask since it's a little spread out over many messages and we can try to frame this in how it related to this SAT fix.

What happened to the connector

We had a connector to the Sentry API that was in Alpha and only supported full refresh syncs. As part of the promotion to Beta, incremental sync support was being added which was released in #20709 . An oncall issue was surfaced in https://github.com/airbytehq/oncall/issues/1317 for a customer saying that despite specifying an incremental sync, they were still seeing all of their records being refreshed on each sync leading to overages.

This connector successfully passed SAT tests during the release. During the investigation, it was discovered that the incremental sync implementation was not working quite right and functionally still replicating all records of streams. It was observed that Sentry syncs were not properly emitting STATE and with the latest synced value, but was instead emitting an empty value {} for each stream. We should have had some way of detecting through SAT that the state was being incorrectly emitted which would have then told us that the implementation needed to be fixed.

Why was Source Acceptance Tests unable to detect this?

Looking specifically at the test_incremental.py test which deals with incremental syncs there are a few things in play. The high level explanation of some of the tests is that we perform an incremental sync. Then, using the latest state, we attempt a second incremental sync and verify that there are no records received, thus verifying that the state was used in the following sync.

What led to the gap was that when we generate the list of RECORD values to compare against the STATE value in records_with_state() to determine that we did not get earlier records, we actually skip over yielding a value when STATE value is none. So in this case where Sentry was emitting an empty dictionary state value, it looks like we were never actually checking any RECORD value > STATE value. In the eyes of the test, this was successful. Even though this is a gap, this might be the correct checking behavior because we can't compare records to state correctly when it is missing.

And this is where we should be doing a better job that STATE is being emitted with an actual value instead of {} for these incremental syncs. And that would have told us that Sentry was not actually emitting a real state message. Although it might be hard to assert on an exact state value because the end of a sync is a product of when SAT is run or the test data in our various integration test accounts. Low effort would be that STATE contains an actual value, not empty, empty string, 0, etc etc. Or if we wanted a more complete solution, we would allow the SAT config to specify a pattern or format that that the state must follow.

As for why test_state_with_abnormally_large_values() was passing. The bug in Sentry was related to the emitting of state. The Sentry connector is adhering to the incoming state correctly and only getting later records. In the context of the OC issue, it was the combination of a first time sync not properly emitting state and so the second sync would receive an empty state and then attempt to sync everything a second time.

This is quite a novela so let me know if this was too much detail or want more clarify on a specific aspect