politics-rewired / Spoke

Politics Rewired's fork of Spoke
GNU General Public License v3.0
35 stars 17 forks source link

chore(config): check test environment for feature flags #1665

Open ajohn25 opened 1 year ago

ajohn25 commented 1 year ago

Description

This modifies the config file to enable feature flags for running tests.

Motivation and Context

https://github.com/politics-rewired/Spoke/pull/1663#issuecomment-1681067348

How Has This Been Tested?

Tests are still passing

Screenshots (if appropriate):

Documentation Changes

Checklist:

ajohn25 commented 1 year ago

they are only meaningful if passed through envalid.cleanEnv()

Got you! Can you say more about what meaningful refers to here? Is it that the validators aren't really validating anymore because they're not being passed through? The failing test for #1663 passed after this commit so i jumped the gun and took that as success 😅

bchrobot commented 1 year ago

they are only meaningful if passed through envalid.cleanEnv()

Got you! Can you say more about what meaningful refers to here? Is it that the validators aren't really validating anymore because they're not being passed through? The failing test for #1663 passed after this commit so i jumped the gun and took that as success 😅

The validator (e.g. bool()) is just a spec for how envalid.cleanEnv() should process an environment variable value. So the typing of const env = envalid.cleanEnv() is:

type CleanEnvOutput = {
  isProd: boolean;
  ALLOW_SEND_ALL: boolean;
};

but the shape of the const config = { ...env, ALLOW_SEND_ALL: bool({ ... }), with your changes would be:

type MixedConfigType = {
  isProd: boolean;
  ALLOW_SEND_ALL: ValidatorSpec<boolean>;
};

The tests likely passed because ValidatorSpec<boolean> is truthy. If the feature flag checked if (config.ALLOW_SEND_ALL === true) {} then a) TS would have complained that ValidatorSpec<boolean> cannot be assigned to boolean or something like that, and b) it would evaluate to false and the unit tests would have failed.

It might be helpful to look at the whole src/config.ts in Switchboard to see how the two interact.

ajohn25 commented 1 year ago

then a) TS would have complained that ValidatorSpec cannot be assigned to boolean or something like that

Got it! Maybe I'll briefly check whether I can easily convert the Spoke config to TS without breaking anything too 😅

bchrobot commented 1 year ago

then a) TS would have complained that ValidatorSpec cannot be assigned to boolean or something like that

Got it! Maybe I'll briefly check whether I can easily convert the Spoke config to TS without breaking anything too 😅

I don't think you need to convert to TS to move forward with this. But I think the types are a good way of understanding what is happening?

ajohn25 commented 1 year ago

I don't think you need to convert to TS to move forward with this. But I think the types are a good way of understanding what is happening?

yeah i agree!