Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test with multiple ORM schemas #629

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon commented Jun 11, 2024

Fixes #618

@vincentchalamon vincentchalamon marked this pull request as draft June 11, 2024 06:36
@vincentchalamon vincentchalamon force-pushed the tests/migrate-multi-schemas branch 2 times, most recently from abcae96 to c3a0265 Compare June 11, 2024 07:19
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for working on this Vincent!

Regarding the failing CI (I'm not familiar with multiple schemas). Do you know what's going on there?

@@ -29,6 +30,11 @@
$fs->remove(__DIR__.'/Fixture/Migrations');
$fs->mkdir(__DIR__.'/Fixture/Migrations');

// restore custom migrations
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Symfony\Component\Filesystem\Filesystem to just copy the entire directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filesystem only allows to copy files, not directories

Copy link
Member

@kbond kbond Jun 12, 2024

Choose a reason for hiding this comment

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

Filesystem::mirror() should do the trick (but this isn't a big deal)

@vincentchalamon
Copy link
Contributor Author

Hi @kbond,

Regarding the failing CI (I'm not familiar with multiple schemas). Do you know what's going on there?

The doctrine:schema:update command doesn't seem to support multiple schemas. To handle such specific use case, I had to separate migrate mode from schema mode fixtures. Push in progress.

This PR is still in WIP till I find a clean solution to test this specific use case.

@vincentchalamon vincentchalamon force-pushed the tests/migrate-multi-schemas branch from c3a0265 to 294a381 Compare June 12, 2024 07:45
@vincentchalamon vincentchalamon force-pushed the tests/migrate-multi-schemas branch from 294a381 to 2755bbc Compare June 12, 2024 07:46
@vincentchalamon vincentchalamon marked this pull request as ready for review June 12, 2024 07:47
@vincentchalamon vincentchalamon requested a review from kbond June 12, 2024 07:47
@kbond kbond requested a review from nikophil June 12, 2024 18:32
@nikophil
Copy link
Member

Hi @vincentchalamon

I'm wondering if a simpler solution would not be to declare two entity managers, affect one entity to this entity manager, and configure the test kernel to not reset this specific EM. WDYT?

@vincentchalamon
Copy link
Contributor Author

Hi @vincentchalamon

I'm wondering if a simpler solution would not be to declare two entity managers, affect one entity to this entity manager, and configure the test kernel to not reset this specific EM. WDYT?

I'm afraid using multiple entity managers won't solve the original issue. The idea is to add non-regression tests to ensure the database is fully and properly removed before running the migrations (cf. #615).

The idea here is to have a custom migration which should fail the tests if not run properly (for instance, if AbstractORMPersistenceStrategy::resetSchema method is updated).

@nikophil
Copy link
Member

ok thanks for explanations, I understand better now!

@nikophil nikophil merged commit 3c31193 into zenstruck:2.x Jun 18, 2024
24 checks passed
@vincentchalamon vincentchalamon deleted the tests/migrate-multi-schemas branch June 18, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Tests] use 2 schemas in the same database
3 participants