SAT: check that previous config schema validates against current connector spec

[alafanechere]

What

Closes #14595
In addition to syntactic checks hardcoded in #15194 we also want to perform classic jsonschema validation against fake previous configuration. This will make sure a configuration created with the previous version of a connector is still compatible with the current version.

How

• Refactor the backward compatibility tests into a single test that is calling: utils.backward_compatibility.SpecDiffChecker.check() airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py
• Refactor the unit tests to make tests cases more readable and maintainable airbyte-integrations/bases/source-acceptance-test/unit_tests/test_backward_compatibility.py
• Add a utils.backward_compatibility.validate_previous_configs to:
  1. Generate fake previous config with the previous connector specification json schema (with hypothesis-jsonschema).
  2. Validate a fake previous config against the actual connector specification json schema.

🚨 User Impact 🚨

Running the unit tests on SAT takes a bit more time as hypothesis generates a lot of fake data for each backward compatibility test case.

Test failure example on SpecDiffChecker.check()

Test failure example on validate_previous_configs()

[alafanechere]

Added this to freeze the version and avoid accidental breaking updates.

[alafanechere]

@pedroslopez I changed this to only compute the diff on the connectionSpecification field of the spec. I think this is more robust as this is in this field that we want to perform the validation, not the other technical field that exists and might be changed.

[alafanechere]

@pedroslopez let me know if you find this structure more readable.
I did not add a lot of new tests cases and actually removed some whose json schema was not valid (making hypothesis-jsonschema fail). (e.g I changed "type": [] to "type": ["string", "string"] etc.)

[pedroslopez]

I feel kind of neutral about it... The transitions vs specifying the params don't seem much different to me - e.g . they both define a before and after spec, a description and in some way indicate if it should fail or not, so I'm not sure what we're gaining with the Transitions.

I suppose one thing that was kind of useful in the previous implementation is that the test cases were separated by what they were testing rather than having them all together in a single list. For example, cases related to verifying if enums have been introduced were grouped together, cases related to required fields were grouped together, etc. Which may be a bit easier to think about but also makes sure we have sufficient tests for a particular problem.

[alafanechere]

I feel kind of neutral about it... The transitions vs specifying the params don't seem much different to me - e.g . they both define a before and after spec, a description and in some way indicate if it should fail or not, so I'm not sure what we're gaining with the Transitions.

You're right there are not much difference, but the two advantages I found are:

• You can use named arguments on data class instantiation; you can't with pytest params. E.G: We can't do pytest.param(previous_spec,currrent_spec, should_fail=True). This is minor, but I found it increases test case readability when you have to scroll through many test cases.
• If we want to serialize the spec tests case to JSON (or any other format) in the future, we could easily add a serialization function to this data class.

Let me know if you think it's overkilled.

I suppose one thing that was kind of useful in the previous implementation is that the test cases were separated by what they were testing rather than having them all together in a single list. For example, cases related to verifying if enums have been introduced were grouped together, cases related to required fields were grouped together, etc. Which may be a bit easier to think about but also makes sure we have sufficient tests for a particular problem.

Yes, you are right. I could split the big list into smaller scoped chunks, but on the other hand, I find it convenient for future maintenance to keep it like this. Let's say you wonder if a transition you consider failing is covered: simply add this transition to the FAILING_TRANSITIONS list, and you won't have to wonder to what list it should be added or create a new dedicated list.

[pedroslopez]

Looks good, nice to see that adding this additional tests ended up being pretty straightforward 😄

I'm not sure I entirely see the benefit of introducing the Transitions class vs using parametrized tests like elsewhere, but I left more details on this in the comments.

Running the unit tests on SAT takes a bit more time as hypothesis generates a lot of fake data for each backward compatibility test case.

How big is the impact here?

[pedroslopez]

Does this fixture no longer return the spec diff?

[alafanechere]

Oops, I did not clean this very well.

[pedroslopez]

Why are we doing this here instead of using the spec_diff fixture?

[alafanechere]

I decided to discard spec_diff fixture for the moment because:

• No other tests are using it.
• The signature of test_backward_compatibility was changed to use actual_connector_spec and previous_connector_spec (required for validate_previous_configs). Using a spec_diff fixture was convenient to have a lighter signature on test_backward_compatibility that only required spec_diff fixture, but now that I added validate_previous_configs, I find it overkill to add a dedicated spec_diff fixture that takes the same inputs as the actual test.

[pedroslopez]

Curious on why the move to a single test case - is this to prevent further checks from running if a previous check fails?

[alafanechere]

I've done it for three reasons:

• Simplifying unit testing: we can create many backward compatible/incompatible specs and throw them against the same function.
• Limiting the growth of the TestSpec class: I preferred to limit the number of lines added to test_core for code readability and defer the actual backward compatibility check logic to a separate module/class.
• Logs readability: Make test outputs more readable by grouping all the backward_compatibility checks in a single test run: in the logs, you'll have something like test_backward_compatibility failed.

Let me know if you think it's not the right direction.

[sherifnada]

when consolidating many unit tests, I think the most important feature to retain is the granularity of the error messages on each assertion, which this class does. So it makes sense with your reasoning. I like it

[pedroslopez]

Transition feels a bit ambiguous, maybe something like SpecTransition or similar would be more appropriate?

[alafanechere]

Renamed in b8be912

[pedroslopez]

I feel kind of neutral about it... The transitions vs specifying the params don't seem much different to me - e.g . they both define a before and after spec, a description and in some way indicate if it should fail or not, so I'm not sure what we're gaining with the Transitions.

I suppose one thing that was kind of useful in the previous implementation is that the test cases were separated by what they were testing rather than having them all together in a single list. For example, cases related to verifying if enums have been introduced were grouped together, cases related to required fields were grouped together, etc. Which may be a bit easier to think about but also makes sure we have sufficient tests for a particular problem.

[pedroslopez]

Oh wait, it seems like the connectors build is failing on a bunch of tests - we should totally fix those:

Results (18.52s):
	      211 passed
	       39 failed
	          - test_backward_compatibility.py:860 test_backward_compatibility[Top level: declaring the required field should fail.]
	          - test_backward_compatibility.py:860 test_backward_compatibility[Nested level: adding the required field should fail.]
	          - test_backward_compatibility.py:860 test_backward_compatibility[Top level: adding a new required property should fail.]
	          - test_backward_compatibility.py:860 test_backward_compatibility[Nested level: adding a new required property should fail.]
	          - test_backward_compatibility.py:860 test_backward_compatibility[Nullable: Making a field not nullable should fail (not in a list).]

.....

https://github.com/airbytehq/airbyte/runs/7694178244?check_suite_focus=true#step:12:8003

[alafanechere]

Oh wait, it seems like the connectors build is failing on a bunch of tests

Should be fixed by f42aa45 🙏

[alafanechere]

Running the unit tests on SAT takes a bit more time as hypothesis generates a lot of fake data for each backward compatibility test case.

How big is the impact here?

I locally ran the unit tests on SAT:

• With the hypothesis tests in 39 seconds (100 fake configs generated for each test case, the default value for hypothesis).
• Without the hypothesis tests in 4 seconds

This will make the Connector Base build take a bit longer in the CI. This duration can be reduced by limiting the number of generated fake configs (50 fake config: 21seconds, 10 fake configs: 4 seconds). If this is a problem we could decide to reduce the number of fake configs only for unit tests but keep 100 for the normal SAT run.

[sherifnada]

very nice!

[sherifnada]

when consolidating many unit tests, I think the most important feature to retain is the granularity of the error messages on each assertion, which this class does. So it makes sense with your reasoning. I like it

[alafanechere]

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/2828112611

Connector	Did it publish?	Were definitions generated?
bases/source-acceptance-test	❌	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

[alafanechere]

The previous publish failed because the generation of fake config took too much time. I reduced the number of generated fake config in 3c5d24d for unit tests.

[alafanechere]

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/2828242749

Connector	Did it publish?	Were definitions generated?
bases/source-acceptance-test	✅	❌

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️