Skip to content
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

Destination acceptence test : improve comparison methods #11579

Merged
merged 19 commits into from
Apr 17, 2022
Merged

Destination acceptence test : improve comparison methods #11579

merged 19 commits into from
Apr 17, 2022

Conversation

DoNotPanicUA
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA commented Mar 30, 2022 •
edited
Loading

What

New destination acceptance tests (#9443) contain more complicated cases. Some destinations require more flexible methods of comparison.

How

Use specific comparison methods instead of comparing all values as text.

Note

The current implementation keeps old comparison methods by default.
To turn on the new DAT tests for a destination, overwrite methods supportBasicDataTypeTest, supportArrayDataTypeTest, supportObjectDataTypeTest and getTestDataComparator.

The AdvancedTestDataComparator.java is the new version of the comparator, which can be extended by a destination-specific version.

Recommended reading order

  1. DestinationAcceptanceTest.java
  2. TestDataComparator.java
  3. BasicTestDataComparator.java
  4. AdvancedTestDataComparator.java

@yurii-bidiuk yurii-bidiuk mentioned this pull request Apr 7, 2022
Merged
40 tasks
@DoNotPanicUA
Move comparison methods to a new class not to overload the test class.
3c911c9 
Basic - old implementation which provides backward compatibility of this PR.
Advanced - comparator required for destinations with enabled DAT tests.
@DoNotPanicUA
format
f28b9af
@DoNotPanicUA
Merge remote-tracking branch 'origin/master' into aleonets/add-dat-te…
af1cff8 
…sts-ph1
@DoNotPanicUA DoNotPanicUA marked this pull request as ready for review April 11, 2022 12:11
@DoNotPanicUA

This comment was marked as outdated.

Sign in to view
@DoNotPanicUA DoNotPanicUA self-assigned this Apr 11, 2022
@DoNotPanicUA DoNotPanicUA linked an issue Apr 11, 2022 that may be closed by this pull request
Run the new DAT(s) against all destinations and find destinations that are not compliant #9444
Closed
@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Apr 11, 2022 •
edited
Loading

/test connector=connectors/destination-mysql

🕑 connectors/destination-mysql https://github.com/airbytehq/airbyte/actions/runs/2150155323
❌ connectors/destination-mysql https://github.com/airbytehq/airbyte/actions/runs/2150155323
🐛

Invalid master version

edgao
edgao reviewed Apr 12, 2022
View reviewed changes
...ava/io/airbyte/integrations/standardtest/destination/comparator/BasicTestDataComparator.java Outdated Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Outdated Show resolved Hide resolved
...ava/io/airbyte/integrations/standardtest/destination/comparator/BasicTestDataComparator.java Outdated Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Outdated Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Outdated Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Show resolved Hide resolved
.../io/airbyte/integrations/standardtest/destination/comparator/AdvancedTestDataComparator.java Outdated Show resolved Hide resolved
@DoNotPanicUA
Copy link
Contributor Author

DoNotPanicUA commented Apr 12, 2022 •
edited by github-actions bot
Loading

/test connector=connectors/destination-mssql

🕑 connectors/destination-mssql https://github.com/airbytehq/airbyte/actions/runs/2154639966
✅ connectors/destination-mssql https://github.com/airbytehq/airbyte/actions/runs/2154639966
Python tests coverage:

Name                                                                                                                            Stmts   Miss  Cover
---------------------------------------------------------------------------------------------------------------------------------------------------
normalization/transform_config/__init__.py                                                                                          2      0   100%
normalization/transform_catalog/reserved_keywords.py                                                                               13      0   100%
normalization/transform_catalog/__init__.py                                                                                         2      0   100%
normalization/destination_type.py                                                                                                  13      0   100%
normalization/__init__.py                                                                                                           4      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/models/airbyte_protocol.py     124      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/models/__init__.py               1      0   100%
/actions-runner/_work/airbyte/airbyte/airbyte-integrations/bases/airbyte-protocol/airbyte_protocol/__init__.py                      2      0   100%
normalization/transform_catalog/destination_name_transformer.py                                                                   155      8    95%
normalization/transform_config/transform.py                                                                                       168     31    82%
normalization/transform_catalog/table_name_registry.py                                                                            174     34    80%
normalization/transform_catalog/utils.py                                                                                           33      7    79%
normalization/transform_catalog/catalog_processor.py                                                                              143     77    46%
normalization/transform_catalog/transform.py                                                                                       45     26    42%
normalization/transform_catalog/stream_processor.py                                                                               524    337    36%
---------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                                            1403    520    63%

@DoNotPanicUA
remove size objects validation. We iterate expected elements and comp…
c2106a4 
…are it with actualObject elements. Expected might have empty elements and they are equal missing element in the actual Object. Also, actualObject might contain additional elements which is not an exception.
@DoNotPanicUA
Merge remote-tracking branch 'origin/master' into aleonets/add-dat-te…
28625d6 
…sts-ph1
@DoNotPanicUA
Merge remote-tracking branch 'origin/master' into aleonets/add-dat-te…
bf9b84c 
…sts-ph1
edgao
edgao approved these changes Apr 16, 2022
View reviewed changes
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for being patient here!

@DoNotPanicUA DoNotPanicUA merged commit 92694c9 into master Apr 17, 2022
@DoNotPanicUA DoNotPanicUA deleted the aleonets/add-dat-tests-ph1 branch April 17, 2022 14:50
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Apr 19, 2022
Merged
suhomud pushed a commit that referenced this pull request May 23, 2022
@DoNotPanicUA @suhomud
Destination acceptence test : improve comparison methods (#11579)
ed1f18d 
* add Boolean, Number, DateTimeWithTZ compare methods

* Improve value comparison

* Compare inherit objects element by element

* Move comparison methods to a new class not to overload the test class.
Basic - old implementation which provides backward compatibility of this PR.
Advanced - comparator required for destinations with enabled DAT tests.

* format

* Move common method to ComparatorUtils. + Review update

* format

* review

* review

* mark resolveIdentifier method as deprecated

* remove size objects validation. We iterate expected elements and compare it with actualObject elements. Expected might have empty elements and they are equal missing element in the actual Object. Also, actualObject might contain additional elements which is not an exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edgao edgao edgao approved these changes

@alexandr-shegeda alexandr-shegeda alexandr-shegeda approved these changes

@sashaNeshcheret sashaNeshcheret sashaNeshcheret approved these changes

@yurii-bidiuk yurii-bidiuk Awaiting requested review from yurii-bidiuk

Assignees

@DoNotPanicUA DoNotPanicUA

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run the new DAT(s) against all destinations and find destinations that are not compliant
4 participants
@DoNotPanicUA @edgao @alexandr-shegeda @sashaNeshcheret