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

Removed ResetDatabase trait #1930

Closed
wants to merge 1 commit into from
Closed

Conversation

SebSept
Copy link
Contributor

@SebSept SebSept commented Mar 29, 2024

Because the tutorial make use of DAMADoctrineTestBundle to reset the database automatically before each test, the ResetDatabase trait is not needed (it may even cause trouble (I guess)).

Because the tutorial make use of DAMADoctrineTestBundle to reset the database automatically before each test, the ResetDatabase trait is not needed (it may even cause trouble (I guess)).
@dunglas
Copy link
Member

dunglas commented Mar 29, 2024

Shouldn't we remove DMADoctrineTestBundle instead for the sake of simplicity?

@SebSept
Copy link
Contributor Author

SebSept commented Mar 29, 2024

I have no real experience using that bundle and no idea of the speed improvements provided.

It's just 2 actions, a composer req command and adding a line in phpunit.xml.
So, not complicated. No more than a minute.

As mentioned on the Symfony documentation, installing DamaDoctrineTestBundle improve the speed and should be encouraged.

So my humble opinion of new comer is to keep DamaDoctrineTestBundle.

@dunglas
Copy link
Member

dunglas commented Mar 30, 2024

Sounds reasonable! @kbond do you have an opinion about this?

@kbond
Copy link

kbond commented Mar 30, 2024

We've made sure this trait works seamlessly with dama. In fact, it's required for the following scenarios:

  • cold run of your tests (database doesn't exist yet)
  • using global state
  • debugging can be a bit easier since you can run a single test w/o dama easily

This isn't to say it's required but if there is a compatibility issue, I'd like to fix.

@SebSept
Copy link
Contributor Author

SebSept commented Mar 31, 2024

So DamaDoctrineTestBundle and Foundry ResetDatabase can look like a useless redundancy but they are not.
Just to mention, Dama ensure database remains untouched after a test runs, while Foundry's DatabaseReset() maker sur that the database is in a certain state.

So the trait should remain in the code snippet.

Maybe we could add a word in the doc to clarify things...

@dunglas
Copy link
Member

dunglas commented Mar 31, 2024

We can close then. Right?

@SebSept
Copy link
Contributor Author

SebSept commented Mar 31, 2024

Yes.
Do you think it worth adding a word in these page ?

@SebSept SebSept closed this Mar 31, 2024
@kbond
Copy link

kbond commented Mar 31, 2024

Just to mention, Dama ensure database remains untouched after a test runs, while Foundry's DatabaseReset() maker sur that the database is in a certain state.

Technically, dama does the same.

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.

None yet

3 participants