[Testing] tmp location in vendor causes annoyance for local development

[askvortsov1]

Bug Report

Current Behavior
I've run into an issue that will be annoying for local development but should not affect automated testing. Currently, the installation setup script stores data from the simulated environment in the tmp directory, and more importantly, the cached config.php, which, as a result of the package's extraction, will now be located in vendor, not with the test source code. So, it will be wiped on every composer update, and as a result, developers will need to re-run composer test:setup whenever they make any composer changes. They will also need to run composer test:setup individually for every flarum extension.

This is problematic because if the database used for tests exists when composer test:setup is run, the setup script will crash, and testing config (which is necessary to run tests) won't be cached. Right now, developers need to completely delete and recreate the test DB every time they want to configure tests for a new database, which is very annoying.

Possible Solution

1. Leave this to extension developers. This may include:
   1. Recommend that developers destroy and re-create the test database between running local tests for various extensions
   2. Recommend that different databases be used for testing different extensions
2. Store the tmp directory for each extension in that extension's test code. We would need to somehow provide the path to this tmp directory to both SetupScript.php (easy to do through each package's setup.php), and to TestCase (not as trivial, perhaps possible through $argv, $argc)
3. Recommend that extension devs install the testing package globally; this makes it much simpler to reuse the same tmp directory for all tests, but makes it more challenging to use separate dbs for separate packages if that's something the developer wants to do. Also, global requirements can get messy very fast, so I don't like this one at all
4. Add support for a composer command as an alternative to test:setup that would confirm that DB credentials work and write a config.php, but not install anything. This would make rerunning test:setup much simpler when a DB already exists, but makes inconsistent state more likely, would prove problematic as core adds migrations, and adds a LOT of unnecessary boilerplate code as we'll need to redo portions of Installation, which also means that if we change that in core, we'll also need to change it in testing.
5. If an env var (FLARUM_TEST_DATA_DIR) exists, use that for the path to tmp. This allows developers to use one shared tmp directory for all packages, which won't be overriden every time composer is updated. However, it doesn't impose anything, as the default tmp would still be used if that env var isn't provided.

My preferred approach is (#)5.
Additional Context
Add any other context about the problem here.

[SychO9]

Option 5 would mean that you either have tmp in vendor or globally, it would make more sense to have it by default in the actual extension, if it's something we can do.

And even if we manage to have the tmp in the actual extension's directory, it would still be nice to solve the database setup problem, as it would be very annoying every time you setup tests for an extension as you mentioned.

For the paths I think we can use the getcwd() function to get the path, since the test script is ran by composer from the composer.json file, I believe it would always return the exact path to the extension's root directory.

For the database problem, one solution could be the ability to pass an option to the command, for example composer test:setup -- --no-migrations and then use that to tell the script whether to populate the database or not. However the core code might not currently allow us to skip the migration step.

[askvortsov1]

Option 5 would mean that you either have tmp in vendor or globally, it would make more sense to have it by default in the actual extension, if it's something we can do.

Do you mean having in the extension in addition to vendor or globally, or in replacement of one of those 2? The main problem with having it in the extension via getcwd() is that we'd be imposing a strict restriction for where that tmp folder should be located (and by extension, how the tests should be arranged) in every extension.

For the database problem, one solution could be the ability to pass an option to the command, for example composer test:setup -- --no-migrations and then use that to tell the script whether to populate the database or not. However the core code might not currently allow us to skip the migration step.

That'd work, and effectively fix the issue, but we'd need to maintain a copy of Installation.php (which can extend Installation.php for simplicity) in flarum/testing without the other steps. We already do this for ExtensionManager, but that's a necessary evil, and I'm not sure whether duplicating additional boilerplate would be a positive change.

My reasoning for going with #5 is that if extension devs want to use one database setup for testing, #5 is the best way to do that. If they want to use different database setups for testing, there won't be a conflict anyway since they'll be using different databases

[SychO9]

That'd work, and effectively fix the issue, but we'd need to maintain a copy of Installation.php (which can extend Installation.php for simplicity) in flarum/testing without the other steps. We already do this for ExtensionManager, but that's a necessary evil, and I'm not sure whether duplicating additional boilerplate would be a positive change.

After checking the code, that'd indeed be the only way, and I agree we should avoid that.

Do you mean having in the extension in addition to vendor or globally, or in replacement of one of those 2? The main problem with having it in the extension via getcwd() is that we'd be imposing a strict restriction for where that tmp folder should be located (and by extension, how the tests should be arranged) in every extension.
...
My reasoning for going with #5 is that if extension devs want to use one database setup for testing, #5 is the best way to do that. If they want to use different database setups for testing, there won't be a conflict anyway since they'll be using different databases

I was thinking in replacement of vendor, but that's a good point, I understand the concern. But if a dev opts to use a different database for an extension, then the issue of the config file getting wiped still remains.

How about this, we go with option 5, but give the ability to specify a path relative to the extension's root directory so that it can be overridden for extension's that use a different database.

Instead of directly reading the value of the env variable, we would first do this:

$path = str_replace('%CWD%', getcwd(), (string) getenv('FLARUM_TEST_DATA_DIR'));

Giving us the ability to do this in phpunit.integration.xml for running the tests:

<phpunit ...>

    ...
    <php>
        <env name="FLARUM_TEST_DATA_DIR" value="%CWD%/tests/integration" force="true" />
    </php>
    ...
</phpunit>

Being able to setup the tests with:

FLARUM_TEST_DATA_DIR=%CWD%/tests/integration composer test:setup

[askvortsov1]

@luceos suggested that for the global location, we could use the OS's tmp folder (or https://www.php.net/manual/en/function.sys-get-temp-dir.php). Should we use that by default vs the vendor location? It'd still be shared across all extensions (unless we incorporate the extension package name into the tmp folder name, which is possible but overengineering IMO)?

I think I prefer the following priority:

A FLARUM_TEST_TMP_DIR_LOCAL env var > a FLARUM_TEST_TMP_DIR env var > the default vendor location

[SychO9]

Isn't that more likely to get cleared for cleanups because it literally contains temporary files ?

I think I prefer the default location to be vendor instead, with the ability to specify a local and a global path. It sounds enough to be honest.

[luceos]

Yeah I think I agree with using vendor and env vars.