FIX: Ensure TempDatabase->build() doesn't tear down the database

[madmatt]

Visiting dev/testsession in a browser and starting a test session doesn't work without this patch, because the temp database is created during TestSessionEnvironment->createDatabase() and then immediately deleted once the request finishes (due to the shutdown function registered in TempDatabase->build().

This PR changes this functionality, so that it does not cleanup the DB upon request exit. This will lead to temp databases lying around, unless you end your testsession, which you should always do anyway (if for no other reason than you would also leave the TESTS_RUNNING.json file lying around)...

I think this is an API change, but I'm not sure how else to get around it, apart from setting it in a project's YML config, which could work, but would also affect other things (e.g. phpunit test runs that use TempDatabase etc)

[blueo]

@madmatt This approach seems pretty reasonable. The nature of a multi-request test means that it would be very difficult to signal what the 'end' of the test is. We've taken the approach of calling testsession/end when we want to cleanup test state. Interestingly enough, there is a argument that leaving test state might not be so bad see https://docs.cypress.io/guides/references/best-practices.html#Dangling-state-is-your-friend as it could help reproduce issues. you could even be a call to dev/tasks/CleanupTestDatabasesTask at the very end to nuke any left over dbs. So perhaps accuracy of db cleanup is a minor(ish) problem

Since this is a change in the current behaviour it might make sense to make that older behaviour configurable in this module? A bit yuck having a config for a config but I guess it is across modules.

Currently I'm setting an env var that changes the config in core. This works for us as the test environment is set up in docker so we can add the var as part of the test setup script pretty easily. But I think this change would be more intuitive for anyone picking up this module for the first time.

[chillu]

@blueo @madmatt I've added you both as contributors to this module, since you have a lot of knowledge and stakes in the project. Feel free to merge where reasonable.

[sminnee]

I just stumbled across this also...

Since this is a change in the current behaviour it might make sense to make that older behaviour configurable in this module? A bit yuck having a config for a config but I guess it is across modules.

The main place where you might not want Matt's change would be a behat test execution, where it calls TestEnvironment from within the behat script rather than pageview.

I would probably add $cleanupOnExit = true to the createDatabase() method, and any other methods in the chain from dev/testsession/start. dev/testsession/start should pass $cleanupOnExit = false (no configuration needed, it's fatally broken unless this setting is provided) and behat can be left as-is.

[sminnee]

Without this being fixed, here's the approach that @blueo has taken I believe:

Set up a config yaml with this:

---
Name: e2etests
Only:
  envvarset: SS_E2E_RUNNING
---
SilverStripe\ORM\Connect\TempDatabase:
  teardown_on_exit: false
---

And then when you want to run tests, set the .env var SS_E2E_RUNNING. With this setting in place, your PHPUnit and/or regular Behat runs are more at risk of leaving junk databases lying around.