Fix overriding original backup upon previous failure

[bbashy]

Only backup .env if it's not equal to the .env.dusk.[env]

This fixes issues when you ctrl+c the php artisan dusk command or it fails somehow without finishing the restore.

[SebastianS90]

What happens if you do cp .env .env.dusk and afterwards php artisan dusk without having a .env.backup file? I think that restoreEnvironment() might fail because it still tries to copy the nonexistent .env.backup back to .env. So the restore should also be skipped if there is no .env.backup file.

[bbashy]

Yeah there's a lot of scenarios for this... a simple exists for the copy of .env.backup to .env?

[themsaid]

Or maybe you can just do @copy(base_path('.env.backup'), base_path('.env')) and @unlink(base_path('.env.backup'))

[bbashy]

Yes, that works? Just ignore any output from that because we don't need to know?

Trying to think of anything else that might bug out...

[SebastianS90]

@bbashy Sure, but I would move the exists and compare logic to backupEnvironment and restoreEnvironment instead of having it in withDuskEnvironment to keep the latter one as simple and comprehensive as possible.

@themsaid isn't @ a bad practice? It might silently ignore other problems, e.g. file permissions.

[bbashy]

Opted for putting another check on the restoreEnv method call.

I'll leave this to see what people think but I think this is the best bet without going overboard.

[SebastianS90]

isn't there one superfluous ) ?

[bbashy]

Fixed :)

[SebastianS90]

Let's hope that will get merged. Afterwards I'll try to get my phpunit.dusk.xml thing from #154 merged. I probably should have made two PRs instead of a combined one 😆

[SebastianS90]

One issue that remains with this approach is the following (less likely) flow:

• Start the dusk tests
• "Oh shit, I forgot to update the CACHE_DRIVER" (or any other setting in .env.dusk)
• Press Ctrl+C to abort Dusk because you want to re-run the tests after the quick configuration fix
• Edit .env.dusk
• Start Dusk again and thereby lose your normal .env configuration

But it's definitely much better than the current behavior.

[georaldc]

Is the file_exists() check on $this->duskFile() needed before restoring the environment? This could actually prevent restoring the backup file if for instance, an error occurs during testing and you try to re-run dusk again after fixing said error because the $this->duskFile() method could now potentially return an unexpected filename string if Dusk uses the existing .env file, which now contains the Dusk configuration ('.env.dusk.'.$this->laravel->environment() would evaluate to ".env.dusk.testing" for instance if your dusk environment value is set to "testing", when the code might be expecting ".env.dusk.local" instead)

[bbashy]

@georaldc You should probably open an issue since this PR didn't change that part of code.