sivakumar-mahalingam opened a new pull request, #4394:
URL: https://github.com/apache/amoro/pull/4394
## Why are the changes needed?
AMS test classes share one AMS instance and one Derby database. Some tests
change or delete shared state, so other test classes fail depending on
execution order. This breaks CI for unrelated PRs (for example #4378 and #4392)
with errors such as `Catalog internal_iceberg_catalog not exists` and `failed
create database`.
There are three root causes:
1. `TestIcebergV3OptimizingPlanning` extends `DerbyPersistence`, and its
`@AfterEach` cleanup truncates the whole shared database. This deletes the
catalogs that the REST catalog tests need.
2. `TestInternalMixedCatalogService` assumes the catalog is empty. If an
earlier test leaves `test_ns` behind, the `listDatabases().isEmpty()` assertion
fails and the nested setup fails with `AlreadyExists`.
3. In `TestInternalIcebergCatalogService$CatalogPropertiesTest`, the
background catalog-scan task can read the old catalog metadata before
`updateCatalog()` finishes and write it back afterward, so the test sees stale
properties.
Close #4393.
## Brief change log
- `TestIcebergV3OptimizingPlanning`
- No longer extends `DerbyPersistence`. It creates a `DerbyPersistence`
only when run standalone (when no `SqlSessionFactory` is initialized yet), so
it does not reset the shared database.
- Tracks the table IDs it creates and, after each test, deletes only the
rows for those tables (from `table_identifier`, `table_runtime`,
`table_process`, and related tables) instead of truncating everything.
- `TestInternalMixedCatalogService`
- In `@BeforeEach`, drops any leftover tables and the `test_ns` database
from earlier tests.
- `TestDatabaseOperation` now checks whether `test_ns` is present or
absent instead of expecting an empty catalog or an exact namespace count.
- `TestInternalIcebergCatalogService$CatalogPropertiesTest`
- After `updateCatalog()`, reloads the catalog through the REST client and
reapplies the update (up to 3 attempts) if the background catalog scan
overwrote it.
- Reads the warehouse value from AMS after the update succeeds.
Only test code changes. There are no production code changes.
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including
negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [x] Run test locally before making a pull request
Ran the affected test classes locally in the order that used to fail
(`TestIcebergV3OptimizingPlanning` first, then the REST catalog tests):
```
./mvnw install -pl amoro-ams -am -Pjava11 -Pspark-3.5
-Djacoco.flink.skip=true \
-Dtest='TestIcebergV3OptimizingPlanning,TestInternalMixedCatalogService,TestInternalIcebergCatalogService,TestDashboardAuthentication,TestAmsServiceMetrics'
\
-Dsurefire.failIfNoSpecifiedTests=false
```
Result: `Tests run: 41, Failures: 0, Errors: 0, Skipped: 0`. `./mvnw
validate` (Spotless and Checkstyle) also passes.
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
