IGNITE-21801 Add missing Schema Sync integration tests #3740
Conversation
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @SuppressWarnings("resource") | ||
| class ItSchemaSyncConfigTest extends ClusterPerClassIntegrationTest { |
There was a problem hiding this comment.
I think the test class is redundant, the configuration framework should test this behavior and there is no need to double-check it in other modules.
There was a problem hiding this comment.
This is an integration test, and it seems usual to test something at a higher level, even if you already tested it using unit tests. Also, this property ('that delayDuration can never be changed') is very important, it's currently a cornerstone for the Schema Sync to work, so a little bit of redundancy here seems to be worth having it.
| return schemaCompatValidator.validateBackwards(row.schemaVersion(), tableId(), txId) | ||
| .thenAccept(validationResult -> { | ||
| if (!validationResult.isSuccessful()) { | ||
| throw new IncompatibleSchemaException(String.format( |
There was a problem hiding this comment.
As a suggestion, we could add a constructor that would accept an error message pattern and varags, inside use org.apache.ignite.internal.lang.IgniteStringFormatter#format.
There was a problem hiding this comment.
I'm not sure such a change makes the situation so much better. The code at the exception creation site becomes just a bit shorter, but the constructor becomes more complex and now has an additional responsability. I would prefer to leave it in its current state.
https://issues.apache.org/jira/browse/IGNITE-21801