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

MySQL source: Add comprehensive data type test #3810

Merged

DoNotPanicUA merged 11 commits into master from aleonets/3562-comprehensive-tests

Jun 7, 2021

Contributor

DoNotPanicUA commented Jun 2, 2021 •

edited

Loading

Issue: #3562

What

We are creating comprehensive integration tests for the MySQL source connector. Such a test guarantees that connector implementation is infallible even in cases when the source contains unexpected/extreme values.
In addition, we provide an abstract implementation of the comprehensive test SourceComprehensiveTest in order to simplify test creation.

How

Prepare a list of cases for testing in issue Create comprehensive data type tests for MySQL #3562.
Implement an abstract comprehensive test in order to simplify the implementation. A common part for the existing acceptance test and the created comprehensive test is moved to a common class SourceAbstractTest.java.
Implement the abstract test with all cases from the issue.

Pre-merge Checklist

Run integration tests

Recommended reading order

MySqlSourceComprehensiveTest.java, CdcMySqlSourceComprehensiveTest.java
SourceComprehensiveTest.java, SourceAbstractTest.java
the rest

DoNotPanicUA requested a review from yaroslav-hrytsaienko

June 2, 2021 10:22

DoNotPanicUA linked an issue

that may be closed by this pull request

Create comprehensive data type tests for MySQL #3562

Closed

yaroslav-hrytsaienko reviewed

View reviewed changes

Contributor

yaroslav-hrytsaienko left a comment

@DoNotPanicUA great work! Needs a bit of tuning.

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+              import java.util.Arrays;
+              import java.util.List;
+              public class DataTypeTest {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA I suggest to change the name of the class. Basically it is not a test it self. Maybe something like test data wrapper, or smth like this would be better.

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+                private static final String DEFAULT_INSERT_SQL = "INSERT INTO %1$s VALUES (%2$s, %3$s);";
+                private final String sourceType;
+                private final Field.JsonSchemaPrimitive airbyteType;

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA please move jsonSchema to a separate class.

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+                  return airbyteType;
+                }
+                public String getName() {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA please rename the method to getNameWithTestPrefix

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+                  return "test_" + testNumber + "_" + sourceType;
+                }
+                public String getCreateSQL() {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA getCreateSQL-> getCreateSqlQuery

Contributor

yaroslav-hrytsaienko Jun 2, 2021

same for others

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+                   * @param insertValue test value
+                   * @return builder
+                   */
+                  public DataTypeTestBuilder addInsertValue(String... insertValue) {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA addInsertValue->addInsertValues

...dard-source-test/src/main/java/io/airbyte/integrations/standardtest/source/DataTypeTest.java Outdated

+                   * @param createTablePatternSQL creation table sql pattern
+                   * @return builder
+                   */
+                  public DataTypeTestBuilder createTablePatternSQL(String createTablePatternSQL) {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA due to JCC createTablePatternSQL->createTablePatternSql

Contributor Author

DoNotPanicUA Jun 2, 2021

fixed everywhere

...ource-test/src/main/java/io/airbyte/integrations/standardtest/source/SourceAbstractTest.java Outdated

+                 * @param testEnv - information about the test environment.
+                 * @throws Exception - can throw any exception, test framework will handle.
+                 */
+                protected abstract void setup(TestDestinationEnv testEnv) throws Exception;

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA setup-> setupEnvironment , testEnv-> environment

Contributor

yaroslav-hrytsaienko Jun 2, 2021

same for others

...-test/src/main/java/io/airbyte/integrations/standardtest/source/SourceComprehensiveTest.java Outdated

+                 *
+                 * @param test comprehensive data type test
+                 */
+                public void addDataTypeTest(DataTypeTest test) {

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA addDataTypeTest -> addDataTypeTestData

...test-integration/java/io/airbyte/integrations/source/mysql/MySqlSourceComprehensiveTest.java Outdated

+                @Override
+                protected void initTests() {
+                  addDataTypeTest(

Contributor

yaroslav-hrytsaienko Jun 2, 2021

@DoNotPanicUA in case of datasets are equal please move them to parent class

Contributor Author

DoNotPanicUA Jun 2, 2021

they are slight different.

yaroslav-hrytsaienko approved these changes

View reviewed changes

DoNotPanicUA requested review from yaroslav-hrytsaienko, subodh1810 and sherifnada

June 2, 2021 14:36

yaroslav-hrytsaienko approved these changes

View reviewed changes

DoNotPanicUA requested a review from tuliren

June 3, 2021 11:06

DoNotPanicUA marked this pull request as ready for review

June 3, 2021 13:05

DoNotPanicUA self-assigned this

DoNotPanicUA changed the title ~~Aleonets/3562 comprehensive tests~~ MySQL source: Add comprehensive data type test

subodh1810 requested changes

View reviewed changes

Contributor

subodh1810 left a comment

Few changes :

We should split the tests. Each data type should have its own individual tests
There should be individual assertion for each data type. We should check the value is correct and not just that the value is present.
We should try to build the tests in such a way that they can be extensible for different JDBC connectors in the future

...-test/src/main/java/io/airbyte/integrations/standardtest/source/SourceComprehensiveTest.java Outdated

+                  final List<AirbyteMessage> recordMessages = allMessages.stream().filter(m -> m.getType() == Type.RECORD).collect(Collectors.toList());
+                  recordMessages.forEach(msg -> LOGGER.debug(msg.toString()));
+                  assertFalse(recordMessages.isEmpty(), "Expected records from source");

Contributor

subodh1810 Jun 4, 2021

I think we should assert proper values instead of just values being present

Contributor Author

DoNotPanicUA Jun 5, 2021

Done

DoNotPanicUA and others added 9 commits

June 5, 2021 12:26


          first test + abstract class for the comprehensive tests


          missing tests


          + migrate common test part from SourceAcceptanceTest to a new abstrac…

…t class SourceTest

+ add test comprehensive for MySQL CDC
+ add javadocs


          clear logging

d307970


          Move common inner classes to public classes.

ec15fc4

+ rename SourceTest to SourceAbstractTest
+ clean autogenerated imports
+ proper builder implementation according to lombok style.


          code autoformatting

76414fb


          [internal review] rename some methods/variables for clarity.

aaabdfd

+ move up `JsonSchemaPrimitive` enum from inner class level


          [internal review] rename TestDataWrapper.java to TestDataHolder.java

02ad48c

+ make test builder without mandatory params (simple builder)


          [3840] add test of unsigned integer value

29b4763

DoNotPanicUA force-pushed the aleonets/3562-comprehensive-tests branch from 562841f to 29b4763 Compare

June 5, 2021 09:36


          Add possibility to set expected values from a streamer. Implement val…

…idation of outcome data.

DoNotPanicUA requested review from subodh1810 and yaroslav-hrytsaienko

June 5, 2021 12:18


          Add value coverage of zero date for CDC stream.

c5845c5

Contributor Author

DoNotPanicUA commented Jun 5, 2021

Few changes :

We should split the tests. Each data type should have its own individual tests

There should be individual assertion for each data type. We should check the value is correct and not just that the value is present.

We should try to build the tests in such a way that they can be extensible for different JDBC connectors in the future

Hi, @subodh1810.
Thank you for your review.
The first implementation was focused on testing that the streamer is infallible even if source data is quite "tricky". But I agree it's not a "comprehensive" test if we don't check a streamer result.
So, I've added the possibility to specify expected values if it's required and corresponding validations.
In addition, I've added all found bugs (from my perspective) to the table.

About your third point. I realize that big difference between JDBC sources and I don't see common data tests for all JDBC sources. So, I decided to create the builder which allows you to describe your tests for any JDBC source. All common operations I moved to the abstract class.
If you have some ideas on how to build common tests for all JDBC sources, please call me at any convenient time for you.

DoNotPanicUA mentioned this pull request

Create comprehensive data type tests for MySQL #3562

Closed

subodh1810 reviewed

View reviewed changes

Contributor

subodh1810 left a comment

@DoNotPanicUA can you run the tests for jdbc sources using /test connector=source-mysql

...t-integration/java/io/airbyte/integrations/source/mysql/CdcMySqlSourceComprehensiveTest.java

+                          .airbyteType(JsonSchemaPrimitive.STRING)
+                          .addInsertValues("null", "1", "127", "-128")
+                          // @TODO returns number instead of boolean
+                          // .addExpectedValues(null, "true", "false", "false")

Contributor

subodh1810 Jun 7, 2021

This should be fixed by the PR #3890

Contributor

subodh1810 commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-mysql

🕑 source-mysql https://github.com/airbytehq/airbyte/actions/runs/914242949
✅ source-mysql https://github.com/airbytehq/airbyte/actions/runs/914242949

subodh1810 approved these changes

View reviewed changes

Contributor

subodh1810 left a comment

Can you create a follow up task to fix the TODOs. Also please run the tests for all the JDBC source connectors using the github command /test connector to make sure the change would not cause any problem. Rest looks good.

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-jdbc

🕑 source-jdbc https://github.com/airbytehq/airbyte/actions/runs/914351340
✅ source-jdbc https://github.com/airbytehq/airbyte/actions/runs/914351340

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-mssql

🕑 source-mssql https://github.com/airbytehq/airbyte/actions/runs/914351570
✅ source-mssql https://github.com/airbytehq/airbyte/actions/runs/914351570

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-oracle

🕑 source-oracle https://github.com/airbytehq/airbyte/actions/runs/914351799
✅ source-oracle https://github.com/airbytehq/airbyte/actions/runs/914351799

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-postgres

🕑 source-postgres https://github.com/airbytehq/airbyte/actions/runs/914352372
✅ source-postgres https://github.com/airbytehq/airbyte/actions/runs/914352372

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-redshift

🕑 source-redshift https://github.com/airbytehq/airbyte/actions/runs/914352793
✅ source-redshift https://github.com/airbytehq/airbyte/actions/runs/914352793

Contributor Author

DoNotPanicUA commented Jun 7, 2021 •

edited by github-actions bot

Loading

/test connector=source-clickhouse

🕑 source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/914353077
✅ source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/914353077

DoNotPanicUA merged commit 213fae1 into master

DoNotPanicUA deleted the aleonets/3562-comprehensive-tests branch

June 7, 2021 11:01

DoNotPanicUA mentioned this pull request

Source MySQL: Connector fails if source contains "tricky" date values #3931

Closed

marcosmarxm mentioned this pull request

Update database connector documentation with new data types tests #4138

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment