You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "terrymanu (via GitHub)" <gi...@apache.org> on 2025/12/28 10:15:16 UTC
[PR] Add SPI-based executor test cases [shardingsphere]
terrymanu opened a new pull request, #37560:
URL: https://github.com/apache/shardingsphere/pull/37560
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add SPI-based executor test cases [shardingsphere]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #37560:
URL: https://github.com/apache/shardingsphere/pull/37560#discussion_r2649611905
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/connector/DatabaseProxyConnectorFactoryTest.java:
##########
@@ -114,4 +139,10 @@ private ShardingSphereDatabase mockDatabase() {
when(result.getProtocolType()).thenReturn(databaseType);
return result;
}
+
+ private static Stream<Arguments> driverTypeProvider() {
+ return Stream.of(
+ Arguments.of(true, Collections.emptyList(), JDBCDriverType.PREPARED_STATEMENT),
+ Arguments.of(false, Collections.singletonList(1), JDBCDriverType.PREPARED_STATEMENT));
+ }
Review Comment:
The test parameters in the driverTypeProvider method are testing two different scenarios (preferPreparedStatement = true with empty parameters, and preferPreparedStatement = false with non-empty parameters), but both expect the same result (JDBCDriverType.PREPARED_STATEMENT). This suggests that the test is not actually verifying the driver type selection logic effectively. Consider adding a test case that expects JDBCDriverType.STATEMENT to validate that the logic correctly differentiates between the two driver types.
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/queryable/export/ExportDatabaseConfigurationExecutorTest.java:
##########
@@ -82,28 +98,64 @@ private Map<String, StorageUnit> createStorageUnits() {
.collect(Collectors.toMap(Entry::getKey, entry -> DataSourcePoolPropertiesCreator.create(entry.getValue()), (oldValue, currentValue) -> oldValue, LinkedHashMap::new));
Map<String, StorageUnit> result = new LinkedHashMap<>(propsMap.size(), 1F);
for (Entry<String, DataSourcePoolProperties> entry : propsMap.entrySet()) {
- StorageUnit storageUnit = mock(StorageUnit.class, RETURNS_DEEP_STUBS);
+ StorageUnit storageUnit = mock(StorageUnit.class);
when(storageUnit.getDataSourcePoolProperties()).thenReturn(entry.getValue());
result.put(entry.getKey(), storageUnit);
}
return result;
}
+ private ShardingRuleConfiguration createShardingRuleConfiguration() {
+ ShardingRuleConfiguration result = new ShardingRuleConfiguration();
+ result.getTables().add(createTableRuleConfiguration());
+ result.setDefaultDatabaseShardingStrategy(new StandardShardingStrategyConfiguration("order_id", "ds_inline"));
+ result.setDefaultTableShardingStrategy(new NoneShardingStrategyConfiguration());
+ result.getKeyGenerators().put("snowflake", new AlgorithmConfiguration("SNOWFLAKE", new Properties()));
+ result.getShardingAlgorithms().put("ds_inline", new AlgorithmConfiguration("INLINE", PropertiesBuilder.build(new Property("algorithm-expression", "ds_${order_id % 2}"))));
+ return result;
+ }
+
+ private ShardingTableRuleConfiguration createTableRuleConfiguration() {
+ ShardingTableRuleConfiguration result = new ShardingTableRuleConfiguration("t_order", "ds_${0..1}.t_order_${0..1}");
+ result.setKeyGenerateStrategy(new KeyGenerateStrategyConfiguration("order_id", "snowflake"));
+ return result;
+ }
+
+ @SneakyThrows({IOException.class, URISyntaxException.class})
+ private String loadExpectedRow() {
+ URL url = Objects.requireNonNull(ExportDatabaseConfigurationExecutorTest.class.getResource("/expected/export-database-configuration.yaml"));
+ return Files.readAllLines(Paths.get(url.toURI())).stream().filter(each -> !each.startsWith("#") && !each.trim().isEmpty()).collect(Collectors.joining(System.lineSeparator()))
+ + System.lineSeparator();
+ }
+
@Test
void assertExecuteWithEmptyDatabase() {
ShardingSphereDatabase database = mock(ShardingSphereDatabase.class, RETURNS_DEEP_STUBS);
when(database.getName()).thenReturn("empty_db");
when(database.getResourceMetaData().getStorageUnits()).thenReturn(Collections.emptyMap());
when(database.getRuleMetaData().getConfigurations()).thenReturn(Collections.emptyList());
- ExportDatabaseConfigurationStatement sqlStatement = new ExportDatabaseConfigurationStatement(null, new FromDatabaseSegment(0, new DatabaseSegment(0, 0, new IdentifierValue("empty_db"))));
- ExportDatabaseConfigurationExecutor executor = new ExportDatabaseConfigurationExecutor();
executor.setDatabase(database);
- Collection<LocalDataQueryResultRow> actual = executor.getRows(sqlStatement, mock(ContextManager.class));
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(
+ new ExportDatabaseConfigurationStatement(null, new FromDatabaseSegment(0, new DatabaseSegment(0, 0, new IdentifierValue("empty_db")))), mock(ContextManager.class));
assertThat(actual.size(), is(1));
LocalDataQueryResultRow row = actual.iterator().next();
assertThat(row.getCell(1), is("databaseName: empty_db" + System.lineSeparator()));
}
+ @Test
+ void assertExecuteWithFilePath() throws IOException {
+ ShardingSphereDatabase database = mock(ShardingSphereDatabase.class, RETURNS_DEEP_STUBS);
+ when(database.getName()).thenReturn("file_db");
+ when(database.getResourceMetaData().getStorageUnits()).thenReturn(Collections.emptyMap());
+ when(database.getRuleMetaData().getConfigurations()).thenReturn(Collections.emptyList());
+ Path tempFile = tempDir.resolve("export-db-config.yaml");
+ executor.setDatabase(database);
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(new ExportDatabaseConfigurationStatement(tempFile.toString(), mock(FromDatabaseSegment.class)), mock(ContextManager.class));
+ LocalDataQueryResultRow row = actual.iterator().next();
+ assertThat(row.getCell(1), is(String.format("Successfully exported to: '%s'", tempFile)));
+ assertThat(new String(Files.readAllBytes(tempFile)), is("databaseName: file_db" + System.lineSeparator()));
Review Comment:
The file read operation at line 156 doesn't specify a character encoding. Consider using Files.readAllBytes(tempFile) with explicit charset conversion like 'new String(Files.readAllBytes(tempFile), StandardCharsets.UTF_8)' to ensure consistent behavior across different platforms.
##########
proxy/backend/core/src/test/java/org/apache/shardingsphere/proxy/backend/handler/distsql/ral/queryable/export/ExportMetaDataExecutorTest.java:
##########
@@ -129,12 +144,26 @@ private ShardingSphereDatabase mockEmptyShardingSphereDatabase() {
@Test
void assertExecute() {
ContextManager contextManager = mockContextManager();
- Collection<LocalDataQueryResultRow> actual = new ExportMetaDataExecutor().getRows(new ExportMetaDataStatement(null), contextManager);
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(new ExportMetaDataStatement(null), contextManager);
assertThat(actual.size(), is(1));
LocalDataQueryResultRow row = actual.iterator().next();
assertMetaData(row.getCell(3), Base64.encodeBase64String(EXPECTED_NOT_EMPTY_METADATA_VALUE.getBytes()));
}
+ @Test
+ void assertExecuteWithFilePath() throws IOException {
+ ContextManager contextManager = mockContextManager();
+ when(contextManager.getComputeNodeInstanceContext().getInstance().getMetaData().getId()).thenReturn("file_instance");
+ Path tempFile = tempDir.resolve("export-metadata.json");
+ ExportMetaDataStatement sqlStatement = new ExportMetaDataStatement(tempFile.toString());
+ Collection<LocalDataQueryResultRow> actual = executor.getRows(sqlStatement, contextManager);
+ LocalDataQueryResultRow row = actual.iterator().next();
+ assertThat(row.getCell(1), is("file_instance"));
+ assertThat(row.getCell(3), is(String.format("Successfully exported to：'%s'", tempFile)));
+ String fileContent = new String(Files.readAllBytes(tempFile));
+ assertMetaData(Base64.encodeBase64String(fileContent.getBytes()), Base64.encodeBase64String(EXPECTED_NOT_EMPTY_METADATA_VALUE.getBytes()));
Review Comment:
The file read operation at line 163 and encoding at line 164 don't specify a character encoding. This could lead to platform-dependent behavior. Consider using StandardCharsets.UTF_8 explicitly for both Files.readAllBytes and String.getBytes() to ensure consistent behavior across different platforms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add SPI-based executor test cases [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu merged PR #37560:
URL: https://github.com/apache/shardingsphere/pull/37560
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org