feat(tests): add test utils (tlib)

[zenoli]

Final version, combining the good stuff from this original draft (V1) and #452 (V3).

A brief overview of what changed:

• set based, nix-native syntax. No more runTests/runTest or describe/it. (Version A from [this comment])(WIP feat(tests): add test utils v3 #452 (comment))
• Arbitrarily nestable tests from V3 by generating rendering the test attr set into a complex bash command that logs nice error messages wit a full trace of the nesting hierarchy.
• Tests automatically respecting the meta.platforms of a wrapper module, if the wrapper name is passed to the test in the first argument (settings.wrapper).
• NEW: Debug flag to generate an executable version of the rendered script for inspection and debugging
• NEW: Support for multi-line assertions and error messages.
• NEW: Shorthand syntax for assertions: Can be provided as a string directly without an error message, in which case an error message, echoeing the failed condition string will be automatically generated.

@BirdeeHub the one thing I did not re-implement from your version is the extend feature. If you want this I will have to study it in more detail. I think it should be possible to add if it is really needed. Just let me know.

• Remove direnv module
• Add a nice way to run tests only for systems it is defined in
• Update CONTRIBUTING.md

[BirdeeHub]

Yes that would be where to pass them

I didnt think about it needing all the ... but it would yeah... pkgs.callPackage only provides stuff you ask for, but for self and now tlib which are passed explicitly, those always get passed even if you don't ask for them, so it either needs both self and tlib arguments, or the ..., or both.

[zenoli]

Yes that would be where to pass them

I didnt think about it needing all the ... but it would yeah... pkgs.callPackage only provides stuff you ask for, but for self and now tlib which are passed explicitly, those always get passed even if you don't ask for them, so it either needs both self and tlib arguments, or the ..., or both.

Yes, I sighed when I saw that, but it was less annoying than anticipated using quickfix list and a small macro :-)

Cool, I will finalize the PR soon then.

[BirdeeHub]

oh actually.

Maybe we could just export tlib from the ci flake? It will warn about that not following the schema, I kinda don't care, but also maybe we could just update { inherit tlib; } into the lib output in the ci flake and then that solves that part too?

Then ppl could just grab self.tlib (or self.lib.tlib) like they currently just grab self.lib for wlib.

Honestly, Im not sure if that is better though. It is just another idea, fully up to you, it doesn't matter to me. Not really sure what the best way to pass them in is, but, that is where they are called, so, whatever way it gets passed would have to go through that same place in some way.

I am completely fine with the way currently shown in the PR for passing them in, so, yeah IDK, just throwing out more ideas.

[zenoli]

Passing the module here would be enough for the platforms guard, but I would also like to grab wrapper.binName for the testname. Hence, I need to apply pkgs here. Do you have an issue with that @BirdeeHub ?

And now since I basically have a default wrapper available, I was thinking of passing it to runTest such that they can use and extend this instead of each runTest having to define a wrapper from scratch using let ... in as I currently do in direnv/check.nix.

Thoughts @BirdeeHub ?

[BirdeeHub]

As far as passing pkgs goes, sure, that is fine. They get the same pkgs given to them.

You should create the test drv with the initial cases/scripts and module they pass and stuff. I like the idea of not passing the module a bunch of times and only passing it once per test drv.

You should allow the helper to also recieve a package if you can, the stuff is exposed under passthru.configuration if you get a package.

You should try to use wlib.makeCustomizable in order to allow them to test.addTest or test.addScript or test.addCase or whatever you wanna call it afterwards too! (Not necessary though. But it would be cool right?) It is a generalization of lib.mkOverrideable that allows that function to be whatever you want.

[zenoli]

I implemented the wrapper-passing-solution on a separate branch, because I am not sure whether I am happy with it:
zenoli#2

The idea is as follows:

runTests self.wrappers.direnv [  # <-- 1. specify the wrapper module here
  (runTest "if nix-direnv is enabled then lib/nix-direnv.sh should exists"
    { nix-direnv.enable = true; }  # <-- 2. specify the test-specific wrapper config
    (wrapper: [  #  <-- 3. receive the wrapper package with the config from (2.) already applied
      (isDirectory (getDotdir wrapper))   # <-- 4. define assertions
      (isFile "${getDotdir wrapper}/lib/nix-direnv.sh")
    ])
  )
  ...

The example above, everything seems pretty clean imo.
Where I feel it gets a bit clunky is if you want to define variables that need to be used in the wrapper config as well as in the assertions:

  (
    let
      libScriptContent = "echo foo";  # needs to be outside of `runTest` as the scope needs to cover the config and assertions
    in
    (runTest "if a lib-script is set then it should be generated" { lib."foo.sh" = libScriptContent; } (
      wrapper:
      let
        libScriptFile = "${getDotdir wrapper}/lib/foo.sh";
      in
      [
        (isDirectory (getDotdir wrapper))
        (isFile libScriptFile)
        (fileContains libScriptFile libScriptContent)

      ]
    ))
  )

Maybe I should scratch the idea of passing the config and simply receive the wrapper (with only pkgs applied) and leave extending the wrapper do the user.

My issue with this is, that I have to invent a new wrapper name:

  (runTest "if nix-direnv is enabled then lib/nix-direnv.sh should exists"  # <-- no more config param here
    (wrapper: 
      let
        wrapper2 = wrapper.wrap { nix-direnv.enable = true; };  # <-- here I need a new name
      in 
        [
          (isDirectory (getDotdir wrapper))
          (isFile "${getDotdir wrapper}/lib/nix-direnv.sh")
        ])
  )

There are of course better ways to than just picking "wrapper2" but it is still odd.

[BirdeeHub]

Ah I see. So, because the whole test owns a copy of the module, but you may want variations of it for each case.

Im not sure. Maybe them owning a copy of it is the wrong abstraction?

Or, you could have each case be allowed to provide its own module if they want? Then the question with that would be, should the next test get the updated state of that, or should it receive the original copy? The user should probably be able to specify that?

Also, you meant this, right?

  (runTest "if nix-direnv is enabled then lib/nix-direnv.sh should exists"  # <-- no more config param here
    (wrapper: 
      let
        wrapper2 = wrapper.wrap { nix-direnv.enable = true; };  # <-- here I need a new name
      in 
        [
          (isDirectory (getDotdir wrapper2))
          (isFile "${getDotdir wrapper2}/lib/nix-direnv.sh")
        ])
  )

[zenoli]

Or, you could have each case be allowed to provide its own module if they want?

Yes, this is the most flexible solution (and basically what I had in my first draft):

(runTest "if nix-direnv is disabled then lib/nix-direnv.sh should not exist" (
    let
      wrapper = self.wrappers.direnv.wrap {
        inherit pkgs;
        nix-direnv.enable = false;
      };
    in
    [
      (isDirectory (getDotdir wrapper))
      (notIsFile "${getDotdir wrapper}/lib/nix-direnv.sh")
    ]
  ))

Maybe I will make runTest able to recieve both a function wrapper -> <listOf Assertion> (where wrapper is pre-configured as in my 2nd draft) but also simply a <listOf Assertions> where they can build their wrapper from scratch using let .. in (as above).

Then the question with that would be, should the next test get the updated state of that, or should it receive the original copy? The user should probably be able to specify that?

I'm not sure if I understand what you mean. I think each runTest should - in some way - provide its own wrapper variant s.t. they stay independent and modular. I wouldn't want a solution where wrappers get successively extended for runTest later in the list, if this is what you meant?
What could make sense if if tests could be arbitrarily nested, and all children inheriting the base wrapper of their parent, but I think this is overkill.

Also, you meant this, right?

Yes, exactly. Forgot to update wrapper2 in the assertions.

All in all, I think the initial solution was the most KISS aproach.
It is flexible, and the syntax is clean and readable (which I think is important for incentivising maintainers to adopt the framework.
The ability with declaratively specifying the config per runTest is nice syntactic sugar.

I will see if I find a solution that keeps the KISS approach by default but allows to opt-in to declarative config solution.

[BirdeeHub]

(runTest "if nix-direnv is disabled then lib/nix-direnv.sh should not exist" (
    let
      wrapper = self.wrappers.direnv.wrap {
        inherit pkgs;
        nix-direnv.enable = false;
      };
    in
    [
      (isDirectory (getDotdir wrapper))
      (notIsFile "${getDotdir wrapper}/lib/nix-direnv.sh")
    ]
  ))

^ I mean, they could already do that with literally any of these solutions, this isn't really what I meant.

This is what you were doing with the wrapper2 thing too.

I more meant, should we be allowing someone to pass an EXTRA module per each individual test case. As in, they would pass a module that extends the base one and we handle that somehow.

That being said, I am starting to think that would be too much.

Maybe I will make runTest able to recieve both a function wrapper -> (where wrapper is pre-configured as in my 2nd draft) but also simply a where they can build their wrapper from scratch using let .. in (as above).

This is not the worst idea I have ever heard, but its still basically just doing wrapper2, and if someone only uses wrapper2 they kinda start to ask "why wrapper1 then"?

If we only need it for name and meta.platforms, should we just have them specify a name for the test, and a list of platforms?

Then the question with that would be, should the next test get the updated state of that, or should it receive the original copy? The user should probably be able to specify that?

^ This part of the statement doesn't make sense without the first part I was trying to say.

That being said, I am not opposed to simpler.

I still need to try this out, I haven't tried it yet. I don't always notice what needs to be done unless I try to use it for something myself.

[zenoli]

I have the opt-in solution almost ready. I will push it soon.

I still need to try this out, I haven't tried it yet. I don't always notice what needs to be done unless I try to use it for something myself.

Yes of course. This would be very valuable. Try it once its ready. We can still strip out the opt-in part if turns out to be unnecessary.

[BirdeeHub]

What if instead of passing in a wrapper module to runTest at all, we just do the runCommand templating you have for runTest, and then expose a mkIfPlatform kinda function that takes the test (as its second parameter) plus any of: (a module), (evaluated module), (config set of evaluated module) (package) or (list of platform strings) as its first parameter, and then returns the (test drv or null) based on the meta.platforms value it has, that they can use at the end to disable it instead of writing out the elem system systems function themselves?

Then they can be in charge of what happens with the module and stuff and we can just focus on making helpers for turning those into tests.

mkTestIfPlatform = cond: test: let
  platforms = if builtins.isList then cond
    else cond.passthru.configuration.meta.platforms or cond.config.meta.platforms or cond.meta.platforms or (self.lib.evalModule cond).config.meta.platforms;
in
if builtins.elem pkgs.stdenv.hostPlatform.system platforms then test else null;

^? something like that?

And then we don't need runTest to accept a module at all really? They can just use the module plus the helpers to construct tests however they want, and then pass whatever they have plus their test to that and return the result?

[zenoli]

I created a separate PR with the changes:
#446

I've also written a section in CONTRIBUTING.md on its usage:

https://github.com/zenoli/nix-wrapper-modules/blob/test-lib-module-passing-rework/CONTRIBUTING.md#writing-tests

This is more or less my proposal as of now.
Maybe give me feedback over there once you get around to testing it?

About your inputs in your latest comment:

What if instead of passing in a wrapper module to runTest at all, we just do the runCommand templating you have for runTest, and then expose a mkIfPlatform kinda function that takes the test (as its second parameter) plus any of: (a module), (evaluated module), (config set of evaluated module) (package) or (list of platform strings) as its first parameter, and then returns the (test drv or null) based on the meta.platforms value it has, that they can use at the end to disable it instead of writing out the elem system systems function themselves?

Then they can be in charge of what happens with the module and stuff and we can just focus on making helpers for turning those into tests.

mkTestIfPlatform = cond: test: let
  platforms = if builtins.isList then cond
    else cond.passthru.configuration.meta.platforms or cond.config.meta.platforms or cond.meta.platforms or (self.lib.evalModule cond).config.meta.platforms;
in
if builtins.elem pkgs.stdenv.hostPlatform.system platforms then test else null;

^? something like that?

And then we don't need runTest to accept a module at all really? They can just use the module plus the helpers to construct tests however they want, and then pass whatever they have plus their test to that and return the result?

Can you give me a "sketch" of your ideas in the form of a review comment somewhere here:
https://github.com/zenoli/nix-wrapper-modules/blob/test-lib-module-passing-rework/CONTRIBUTING.md#writing-tests

Just like how it would look like to specify the tests with those ideas - to have a better basis for discussion?

[BirdeeHub]

So, I will hopefully get an answer back to you about this today or tomorrow.

I have a slight addition to v2

basically just this for expected instead of regular toJSON.

  toSanitizedJSON =
    value:
    if builtins.isAttrs value then
      builtins.toJSON (
        lib.mapAttrsRecursive (
          path: v:
          if builtins.isFunction v then
            let
              id = toString (
                builtins.unsafeGetAttrPos (lib.last path) (
                  lib.getAttrFromPath (lib.sublist 0 (builtins.length path - 1) path) value
                )
              );
            in
            "<function${if id != "" then " ${id}" else ""}>"
          else
            v
        ) value
      )
    else
      builtins.toJSON value;

---

And I have another direction we could take it much like the first version which didn't accept the wrapper module as an argument, but with some slight differences. But I have been busy the last few days and haven't found the time to give a well thought-through reply.

I hope to get you an example implementation and usage of what I was talking about for that other direction sometime today though. We will see.

I will need to fix #447 and #449 first though.

[zenoli]

Thanks for the sanitize fn. I think I know what it does as I yesterday noticed it cannot transform arbitrary nix sets.

[zenoli]

So, I will hopefully get an answer back to you about this today or tomorrow.

You don't need to hurry. I again generalized the v2 approach a lot yesterday to also make it suitable for non-wrapper tests but general ones (like core/types/etc).

Better wait until I pushed this and I will ping you once it ready.

@BirdeeHub (pinging you now too, to make sure you see it and not waste time reviewing outdated code :-))

That said, the changes are really just a generalization of the current v2: It generalizes the conditional execution for platforms to generic conditions, and the passing of config (which gets transformed to a configured wrapper) to passing generic contexts

[BirdeeHub]

I am much happier with this design.

[BirdeeHub]

the one thing I did not re-implement from your version is the extend feature. If you want this I will have to study it in more detail. I think it should be possible to add if it is really needed. Just let me know.

^ that's fine. It was kinda overkill and of limited utility. But, wlib.makeCustomizable is pretty cool, you should check it out still XD Don't bother figuring out how it works completely, trust me I only just barely managed to understand it when I wrote it.

It calls the thing, and adds the function back to the result of the thing so you can call it again basically, and it merges the args for the calls.

It works like lib.mkOverrideable with 2 extra args and it lets you customize how the args get merged from one call to the next, thats all you really need to know to use it XD

well, that, and if you add more on top, you should set patches in the settings (second arg) so the previous ones don't remove the new ones. lib.mkOverrideable doesnt have a way to solve that, so that is a new concept thing to that function

It could always be added later if we do want it.

[zenoli]

^ that's fine. It was kinda overkill and of limited utility. But, wlib.makeCustomizable is pretty cool, you should check it out still XD Don't bother figuring out how it works completely, trust me I only just barely managed to understand it when I wrote it.

Oh I did check it out already and yes it is pretty cool. I honestly was just to lazy (and exhausted) to re-implement it because the cleanup turned out to be more work than anticipated and wanted to call it a day. I think we should still be able to add in in a non-breaking way, should we ever have a need for it.

[zenoli]

@BirdeeHub thanks for the review. I implemented you're ideas. Can you check again?

[BirdeeHub]

Suggested change

	If only `settings.wrapper` is set, the name will be derived from this value by suffixing it with `-test`.
	If only `settings.wrapper` is set, the name will be derived from this value by suffixing it with `-test`.

[BirdeeHub]

Suggested change

	If `settings.name` is set, it will be taken as the name of the derivation.
	If `settings.name` is set, it will be taken as the name of the derivation.

[BirdeeHub]

Just fixes to the nixdoc args and some (minor) formatting nitpicks left I think!

nix run ./ci#docs -- ./_site && cd _site && python3 -m http.server 1337 it and go to https://localhost:1337 to check it out, it is pretty fast to build tbh.

The mdbook module is pretty nice 😸

[BirdeeHub]

wait what changed to make it fail?

[BirdeeHub]

oh the fish module got added and the checks for it need ...

[BirdeeHub]

I added ... to the fish tests now :) You might have to rebase on top of that somehow

[BirdeeHub]

Also I apparently forgot a semicolon in my suggestion for enabled (which I did not test btw)

[zenoli]

@BirdeeHub don't worry, I'm still updating stuff :-)

[zenoli]

About the enable option:

If I run a wrapper test on an unsupported platform, the test should not be run - independent of the enable flag, right?

[BirdeeHub]

About the enable option:

If I run a wrapper test on an unsupported platform, the test should not be run - independent of the enable flag, right?

If I do enable = false I want it to be disabled. I don't care what platforms. Just disabled.

Otherwise, it goes based on platform, or is enabled if not a wrapper test.

So, in this case, enable does nothing unless it is false, in which case, it disables the test for sure.

[BirdeeHub]

https://github.com/BirdeeHub/nix-wrapper-modules/pull/431/changes

Also ^ this is why the enable option needs to disable and why I want it, for context. (basically, claude-code...)

[zenoli]

Ok, I think I'm done with the changes.

Rebased & squashed + removed direnv module.

Anything else @BirdeeHub ?

[BirdeeHub]

No I think that is it! I think you did a good job with these!

Will wait for the mac tests to run.

Github is being crazy today, it has been very hard for me to use it I have had to pull every single thing locally.

[zenoli]

Thanks! And thanks for the various inputs. Took more work than initially anticipated, but learned a lot about nix along the way :-)

Mac tests are through.

[zenoli]

Github is being crazy today, it has been very hard for me to use it I have had to pull every single thing locally.

FYI: https://www.githubstatus.com/incidents/x69zbgdyfzg0