Open sarayourfriend opened 3 months ago
I removed these features from the preparePageForTest
completely in #8048. This way, we don't set up all of the switchable flags by default. Instead, we only set the cookie value for a flag that we explicitly add to the preparePageForTest
parameters. All other values stay as default (i.e., they use the defaultValue
of the flag, without specifically setting its preferredValue
).
This better matches the behavior in prod: if we don't set any values on the /preferences
page, we don't have any preferredValue
s for any flags, and thus we use the defaults.
I think we should also update the featureFlagStore
to only save the preferred state of the flag if it was explicitly toggled using a switch (on /preferences
for most, and in the sidebar for fetch_sensitive
). Currently, whenever a user toggles a switchable feature, we save a cookie with all switchable flag states.
@obulat I feel like I'm missing some necessary context to your comment. Can you clarify what you're talking about and how it affects or relates to the changes I've proposed in the issue?
Current Situation
The playwright environment currently hard-codes default feature flag states in a non-obvious location, the navigation utilities:
https://github.com/WordPress/openverse/blob/3ed38fc4b138af2f6ac03fcc065ec633d6905d73/frontend/test/playwright/utils/navigation.ts#L207-L213
Suggested Improvement
It would be more intuitive to move these defaults into
feature-flags.json
itself. We can do this by adding a new deployment environment,testing
.Steps to implement:
testing
, betweenlocal
andstaging
in theDEPLOY_ENVS
array . It will never affect live environments because those start atstaging
and move up toproduction
, never looking "backwards" in theDEPLOY_ENVS
array.feature-flags.json
using the newtesting
deployment environment.DEPLOYMENT_ENV
totesting
in the pnpm test scripts infrontend/package.json
.feature-flags.json
for the default state. If it isn't possible to use the feature flag store in test directly, extract the function to derive the default state for an environment into a utility function accessible by tests that accepts the feature flag JSON and the deployment environment as arguments. This function can then be used by the feature flag store and the Playwright navigation utility to ensure the method used to derive the default state is identical and maximise the DRY-ness of the code.The PR must have:
just p frontend test
.just p frontend test:unit
,just p frontend test:playwright
,just p frontend test:storybook
will run the unit, playwright, and storybook tests respectively.just lint
.navigation
, so theoretically, aside from changing the navigation utility to use the feature flag JSON (or pull from the feature flag store, if possible), there should be no other changes to tests.Benefit
An identical method for deriving feature flag state is identical in all execution environments is clearer, more intuitive, and easier to understand at a glance. It also makes it more likely that we will clean up these default states in tests at the same time (or ideally before) we roll features out to production.
That way when adding or modifying a feature, it's immediately clear what is and is not being testing "by default" in our e2e test suite.
Additional context
Thought of this while reviewing https://github.com/WordPress/openverse/pull/4084. The PR doesn't introduce this issue, it just moves code around I'd never seen before, so it isn't something that PR should address.
An alternative approach I considered is to add a separate, optional key to the feature flag configuration object named
defaultStateInTest
that can either bedisabled
orenabled
. @WordPress/openverse-frontend do y'all have a preference between the two options? I think adding a new deployment env and changing our test execution to setDEPLOYMENT_ENV=testing
would be the cleaner approach, but I could see it being a concern to add a new artificial deployment environment for testing.