stylix icon indicating copy to clipboard operation
stylix copied to clipboard

stylix: test the global enable option using assertions

Open danth opened this issue 8 months ago • 5 comments

This adds a pair of assertions to each testbed: that the system is not changed in any way when Stylix is disabled, and that it is changed when Stylix is enabled.

I originally wanted this to compare the value of every option, but that uncovered a variety of errors in upstream code which is not normally evaluated - things like meta.maintainers definitions using undefined variables. I realised it would also require filtering out certain subtrees, such as the stylix.enable option itself, which we expect to change.

In the end, I opted to just compare the toplevel derivations instead.

Things done

Maintainer CC

danth avatar Mar 26 '25 15:03 danth

One thing to note is that the "stylix should affect the system when enabled" assertion doesn't know which module that effect is coming from: it's not testing that the module related to the testbed does something, but rather that any module does something.

We could potentially improve this by adding an extra system configuration, where the module in question is added to disabledModules, and then ensure something changes compared to the normal enabled configuration.

The main purpose of this PR is to check "stylix does not affect the system when disabled", so that could be left for later.

danth avatar Mar 26 '25 15:03 danth

Seems to be failing due to memory constraints.

I'll finish #947 first, as that should help with this, and then rebase.

danth avatar Mar 26 '25 15:03 danth

Not sure if this is actually a good idea, as it triples the cost of evaluating each testbed. We also need to mock out certain options to be able to import the testbed file while not importing Stylix.

danth avatar Mar 30 '25 15:03 danth

For reference, this PR would partially automate the process that would resolve https://github.com/danth/stylix/issues/543#issuecomment-2323517576. It is "partially automate" because not all targets and options are covered by testbeds.

Not sure if this is actually a good idea, as it triples the cost of evaluating each testbed. We also need to mock out certain options to be able to import the testbed file while not importing Stylix.

What about introducing a new testbed that composes all target testbeds, with the following variants:

  • clean: Not importing Stylix
  • enabled: enable = true;
  • disabled: enable = false;

The heuristic for selecting testbeds would be to prefer /modules/<MODULE>/testbeds/default.nix over any other /modules/<MODULE>/testbeds/<TESTBED>.nix testbed. If there is no /modules/<MODULE>/testbeds/default.nix, then the testbed could be chosen by something arbitrary like:

builtins.head (
  lib.naturalSort (
    builtins.attrNames (builtins.readDir /* /modules/<MODULE>/testbeds */)
  )
)

trueNAHO avatar Mar 31 '25 15:03 trueNAHO

The main issue is that the clean copy doesn't have any of the stylix.* options defined, and some testbeds contain things like stylix.targets.firefox.firefoxGnomeTheme.enable = true which then fails. To add mock versions of the options, we have to introduce a fourth copy since doing this to clean would conflict with the real options when we define them.

danth avatar Mar 31 '25 15:03 danth