Skip to content

Conversation

@dennisvang
Copy link
Contributor

@dennisvang dennisvang commented Nov 13, 2025

Replace SQL test data migrations by JSON test data fixtures.

  • remove reference to test data migration files from flyway test config
  • replace test data migration files (.sql) by test data fixture files (.json) (extend default fixtures, do not duplicate)
  • specify test data location in bootstrap test config (depends on Enable loading of relational db fixtures from multiple directories #793)
  • get repository populator to run again before each test (because flyway clean is called before each test)
  • check if tests run slower due to fixtures (compared with the original migration approach) -> approx. 20 min on macOs and Windows, 11 min on Linux, very similar to the original sql migration implementation

@dennisvang dennisvang changed the title Remove sql test data migrations Replace SQL test data migrations by JSON test data fixtures Nov 13, 2025
@dennisvang dennisvang changed the title Replace SQL test data migrations by JSON test data fixtures Bootstrap test data from JSON fixtures instead of SQL migrations Nov 13, 2025
@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch from 4d9f371 to 0a80643 Compare November 13, 2025 16:56
@dennisvang dennisvang self-assigned this Nov 13, 2025
this is more consistent with e.g. the 'locations' option for flyway
This allows us to include the prefix in the config, which is more flexible.
For example, we can set file:fixtures for the default fixtures,
which need to be overridable in the docker container,
and we can set classpath:test-fixtures for the test fixtures,
which can then be included in the test/resources dir.

Moreover, this approach is similar to the way flyway.locations are specified.
the populator is done, but that does not necessarily mean any repositories were actually populated
* include root logger AppenderRef, so we only need to change the log level, when required
* rename test logging config file for clarity and conformance to log4j2 best practices
@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch from 0739c42 to caddb3d Compare December 8, 2025 14:13
@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch from dd7d2df to a1e6c8c Compare December 8, 2025 16:15
@dennisvang
Copy link
Contributor Author

dennisvang commented Dec 9, 2025

For testing it would be convenient to have the ability to select specific fixtures files or filter fixture files.

We should allow passing complete ant-style patterns.

This way we could do things like "file:fixtures/02*.json" to include all fixtures in the 02xx range.

Also see PathMatchingResourcePatternResolver and AntPathMatcher for examples.

@dennisvang
Copy link
Contributor Author

dennisvang commented Dec 9, 2025

In case we ever need to override or extend the default bootstrap locations from application-testing.yml, we can annotate the test class as follows:

@TestPropertySource(properties = {
        "bootstrap.locations[0]=file:fixtures",
        "bootstrap.locations[1]=file:some/other/location"
})

Note that this overrides the default location list, instead of extending it, so we need to add the default locations explicitly if needed.

@dennisvang
Copy link
Contributor Author

dennisvang commented Dec 9, 2025

Warning

Apparently there are foreign key cascades in place for objects related to user accounts (and probably other relations as well?). As a result, when a fixture replaces an existing record (e.g. from a fixture applied earlier) by using the same uuid, all related objects are removed automatically. For example, adding a test fixture that modifies the default Albert Einstein user account causes all existing api-keys and saved-queries for that user to be deleted silently.

this enables us to specify simple directories, specific files, filters like 'fixtures/02*.json', and wildcards like 'fixtures/**/*.json'
the Path methods caused errors on windows if the location included ':' or '*' characters
description said Nikola Tesla, but uuid was for Albert Einstein
…ure data

changed uuid and type for SearchSavedQuery objects to minimize interference with the existing acceptance tests
Due to the addition of DatabaseBootstrapTests, fixture 0130_test-users-with-api-keys-and-saved-queries.json now includes two new SearchSavedQuery objects.
One of these new objects replaces the other, so the total number of SearchSavedQuery objects expected in the test database is increased by one.
@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch from 05d7172 to ce810ea Compare December 10, 2025 12:21
Although I prefer .org, existing docs and tests expect .com.
Also it will likely lead to confusion because people will keep trying to log in using the .com addresses.
@dennisvang dennisvang marked this pull request as ready for review December 10, 2025 14:04
@dennisvang dennisvang mentioned this pull request Dec 10, 2025
1 task
@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch 2 times, most recently from de8c19c to 7682cd4 Compare December 16, 2025 11:25
// re-populate the database using fixtures
populator.populate(new Repositories(applicationContext));
// re-migrate acl data
// (TODO: AclMigration is in a subfolder of rdf/migration, but is it even related to rdf? Looks relational...)
Copy link
Contributor

@MarekSuchanek MarekSuchanek Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is indeed strange... it was probably there due to close relation to RDF records (it specifies access to an RDF records, but the ACL itself is relational - used to be in Mongo previously).

@TestConfiguration
public class MetadataTestConfig {

// TODO: Looks like we can remove this class altogether.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to move the annotation cleanup to a separate PR because there's more than expected.
I reset this branch by a few commits and did a force-push, which is a bit messy, sorry for that.

@dennisvang dennisvang force-pushed the proposal/634-remove-sql-test-data-migrations branch from 7682cd4 to 2341d09 Compare December 16, 2025 12:50
@dennisvang dennisvang merged commit a3c18ec into feature/634-boostrapping-fdp Dec 16, 2025
24 checks passed
@dennisvang dennisvang deleted the proposal/634-remove-sql-test-data-migrations branch December 16, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants