Conditional theme settings and split theme-settings unit tests into separate suites

[albchu]

Whats in this

https://vault.shopify.io/gsd/projects/44140-Conditional-Settings-for-Themes

Few changes here:

• I split apart all the theme-settings unit tests from the 800 line spec file into separate suites by what each describe block was testing. Most notably the color_scheme_group unit tests were split into its own suite. I build upon this suite for conditional settings.
• Added validation for conditional settings. Specifically added it for all approved settings types and added unit tests for both approved and explicitly unsupported settings types
• color_scheme_group nested settings must not allow conditional settings. This is also now applied and unit tested.

Closes https://github.com/Shopify/shopify/issues/572105

[graygilmore]

👍🏻 test refactors make sense to me though I can't comment on the schemas/theme/setting.json changes

[six-shopify]

Looks good to me, nice work! A few questions/suggestions, but you're much more of a maintainer for this repo than I am so I think it's your call.

[six-shopify]

Not sure how feasible this is so it's just a question — but rather than

for each forbidden setting type, test that the diagnostic correctly forbids it

maybe it should be

gather the diagnostic result for every setting type, and check that the result list contains exactly the list of forbidden types

I feel like that's maybe testing a more useful property. (I know you already have the handwritten schemas with allowed conditional settings, but if there's a list of all settings types at hand somewhere this could bolster the handwritten stuff with a broader property-based test.)

[albchu]

This is an interesting way to look at it! I had considered an approach similar to what you have in mind, but the issue came down to consciously handwriting invalid settings in with our valid ones. My concern was that it would make that fixture a bit too difficult to scan and compromises its value as a self documenting example of valid settings.

The approach I opted for instead is to include all valid setting types that support conditional settings within the theme-block-conditional-settings.json fixture and then only targeting the ones we explicitly do not wish to support (as listed in Clauderic's decision figjam).

[six-shopify]

worth lifting to a shared file within the tests dir since it's repeated multiple times?

[six-shopify]

maybe worth lifting this to the same shared file. and i guess this would be the list of all settings types that could potentially be validated against the set of settings types not supporting visible_if ?

[six-shopify]

So quite a lot of the tests that were added look like this:

const result = doSomething();
expect(result).toStrictEqual(
  /* some object with an exact shape including some specific message */
);

These are basically just snapshot tests, and to ease writing and maintenance I'd ask if we can't just use the snapshot API. It's not super well known, but snapshots can use expect.any(Number) matchers just like you're doing here (that link is towards the Jest docs since they're more detailed, but Vitest supports the same functionality).

The benefit is that if a diagnostic message changes, we can just update the snapshots automatically and then only need to validate the git diff. It also makes it a lot quicker to add new test cases while sticking to the existing pattern. Up to you, though!

[albchu]

Im scared of making too many drastic changes to the existing unit tests that I refactored so I will be avoiding this one simply to limit the churn Ive done here 😅