MySQL source: Add comprehensive data type test #3810
Conversation
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class DataTypeTest { |
There was a problem hiding this comment.
@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.
| 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; |
There was a problem hiding this comment.
@DoNotPanicUA please move jsonSchema to a separate class.
| return airbyteType; | ||
| } | ||
|
|
||
| public String getName() { |
There was a problem hiding this comment.
@DoNotPanicUA please rename the method to getNameWithTestPrefix
| return "test_" + testNumber + "_" + sourceType; | ||
| } | ||
|
|
||
| public String getCreateSQL() { |
There was a problem hiding this comment.
@DoNotPanicUA getCreateSQL-> getCreateSqlQuery
There was a problem hiding this comment.
same for others
| * @param insertValue test value | ||
| * @return builder | ||
| */ | ||
| public DataTypeTestBuilder addInsertValue(String... insertValue) { |
There was a problem hiding this comment.
@DoNotPanicUA addInsertValue->addInsertValues
| * @param createTablePatternSQL creation table sql pattern | ||
| * @return builder | ||
| */ | ||
| public DataTypeTestBuilder createTablePatternSQL(String createTablePatternSQL) { |
There was a problem hiding this comment.
@DoNotPanicUA due to JCC createTablePatternSQL->createTablePatternSql
There was a problem hiding this comment.
fixed everywhere
| * @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; |
There was a problem hiding this comment.
@DoNotPanicUA setup-> setupEnvironment , testEnv-> environment
There was a problem hiding this comment.
same for others
| * | ||
| * @param test comprehensive data type test | ||
| */ | ||
| public void addDataTypeTest(DataTypeTest test) { |
There was a problem hiding this comment.
@DoNotPanicUA addDataTypeTest -> addDataTypeTestData
|
|
||
| @Override | ||
| protected void initTests() { | ||
| addDataTypeTest( |
There was a problem hiding this comment.
@DoNotPanicUA in case of datasets are equal please move them to parent class
There was a problem hiding this comment.
they are slight different.
DoNotPanicUA
requested review from
yaroslav-hrytsaienko,
subodh1810 and
sherifnada
DoNotPanicUA
changed the title
There was a problem hiding this 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
| 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"); |
There was a problem hiding this comment.
I think we should assert proper values instead of just values being present
…t class SourceTest + add test comprehensive for MySQL CDC + add javadocs
+ rename SourceTest to SourceAbstractTest + clean autogenerated imports + proper builder implementation according to lombok style.
+ move up `JsonSchemaPrimitive` enum from inner class level
+ make test builder without mandatory params (simple builder)
DoNotPanicUA
force-pushed
the
aleonets/3562-comprehensive-tests
branch
from
562841f to
29b4763
Compare
…idation of outcome data.
Hi, @subodh1810. 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. |
There was a problem hiding this comment.
@DoNotPanicUA can you run the tests for jdbc sources using /test connector=source-mysql
| .airbyteType(JsonSchemaPrimitive.STRING) | ||
| .addInsertValues("null", "1", "127", "-128") | ||
| // @TODO returns number instead of boolean | ||
| // .addExpectedValues(null, "true", "false", "false") |
There was a problem hiding this comment.
This should be fixed by the PR #3890
|
/test connector=source-mysql
|
|
/test connector=source-jdbc
|
|
/test connector=source-mssql
|
|
/test connector=source-oracle
|
|
/test connector=source-postgres
|
|
/test connector=source-redshift
|
|
/test connector=source-clickhouse
|
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
SourceComprehensiveTestin order to simplify test creation.How
SourceAbstractTest.java.Pre-merge Checklist
Recommended reading order
MySqlSourceComprehensiveTest.java,CdcMySqlSourceComprehensiveTest.javaSourceComprehensiveTest.java,SourceAbstractTest.java