DEV: Introduce `TemporaryRedis` and unset `DISCOURSE_*` env vars in the `themes:isolated_test` rake task (PR #13401)

Because this rake task can be called in production (assuming all the required dependencies are installed), many DISCOURSE_* env variables will need to be unset like the ones related to CDN/S3 so that the Unicorn server that will be spawned for the tests doesn’t pick them up and break the tests.

GitHub

To be honest this scares me a bit. I feel like trying to keep track of which variables we’ve unset is going to be a nightmare in the long term and the source of many bugs. A developer might be completely surprised when their code that depends on a variable suddenly isn’t working.

I understand @SamSaffron has reviewed this approach in another PR, but I wanted to make sure we’ve considered every other possible approach before going this way.

If we printed a message to the console to indicate some env vars have been unset, maybe it would make users quickly understand what’s going on when they run into a weird error/bug caused by a missing variable? We could also print the name of every variable we unset? Or make this behavior opt-in?

I like the idea of outputting the variables that are unset. It could be a warning for each one.

Opt in could also a great idea. Do you think most people would need this? If not let’s make it opt in!

Honestly I’m not sure if anyone uses this task :sweat_smile: It creates/migrates a temporary database, installs the themes on that database and then runs the tests of the themes. In most cases, people just use themes:install and themes:qunit which work on an existing database (which I imagine what most people want in a dev environment).

The only use case of this task is to run a theme/component tests in production when a site is deployed, and to abort the deploy if the tests fail. In this situation we can’t use the production database because we have to update the theme before running the tests. There are also some dependencies (Chrome, Yarn, gems in development/test groups) you’d need to install in production before calling this task because these dependencies are not installed in our official docker image. Internally, we use this for only one of our customer’s staging site and that’s the only use of this task that I’m aware of.

Anyways, will make this opt-in so it becomes extra obvious when invoking this task!

1 Like

Sounds good to me!

@OsamaSayegh I kind of feel the same way as @eviltrout here where it scares me that we’re altering production env keys and also attempting to create a temporary database in the production environment. IMO, any tests that we intend to run as part of the build process needs to be isolated from the production environment and only be executed in the test environment. What are some of the challenges in running the tests in our test docker image?

Given this is opt-in now I am approving this.

The title of this pull request changed from “DEV: Unset most DISCOURSE_* env vars in the themes:install_and_test rake task” to "DEV: Introduce TemporaryRedis and unset DISCOURSE_* env vars in the themes:isolated_test rake task