Skip to content

refactor(tests): move helper module tests to their relevant helper module#438

Merged
BirdeeHub merged 1 commit into
mainfrom
tests
Apr 15, 2026
Merged

refactor(tests): move helper module tests to their relevant helper module#438
BirdeeHub merged 1 commit into
mainfrom
tests

Conversation

@BirdeeHub
Copy link
Copy Markdown
Owner

@BirdeeHub BirdeeHub commented Apr 15, 2026

@zenoli you were mentioning test improvements?

This is the thing where they are allowed to return a set of tests if they want rather than just 1

It also calls them with callPackage thecheck.nix { inherit self; } instead of import thecheck.nix { inherit pkgs self; } now, which is nicer to use.

@BirdeeHub BirdeeHub force-pushed the tests branch 6 times, most recently from be51ab4 to 3618ae9 Compare April 15, 2026 06:08
@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Apr 15, 2026

Thanks. I plan to look at this tonight.
In my direnv branch I am no longer relying on multiple tests, as in "multiple tests per module that can be ran independently". My solution there simply allows for running multiple tests within the same runCommand.

I listed some points why I ended up not needing them here:

#378 (comment)

I will try your branch and see if my points on not needing multiple tests still hold :-)

Will this still be possible to run all tests of a given module?

nix build ./ci#checks.<system>.thatone

@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Apr 15, 2026

Also are you fine with a PR where I extract the test utils from the direnv branch into the ci flake?
I am working on some wrappers where I would also want to use them.

@BirdeeHub BirdeeHub force-pushed the tests branch 3 times, most recently from 52cd915 to db8ae43 Compare April 15, 2026 06:24
@BirdeeHub
Copy link
Copy Markdown
Owner Author

BirdeeHub commented Apr 15, 2026

nix build ./ci#checks.<system>.wrapperModule-thatone will only run the checks if it provides a single derivation

Otherwise if you were returning a set it would be

nix build ./ci#checks.<system>.wrapperModule-thatone-attrname

But, theoretically if you had multiple for your module, that would be what you want, and otherwise you wouldn't care?

It is something that should mostly be used by helper modules to be honest. The nix command + flake schema doesn't really give us a good way to group tests.

I think there is a flake schemas thing that is in determinate nix, added about the same time as wasm plugins, that we could use when it gets upstreamed, that could allow us to define what the checks output looks like ourselves and what happens with it? That would hopefully allow nested and grouped checks and we could revise it then?

But I wanted to move the tests for the helper modules to the helper modules, and I didn't want to group them all into 1 big derivation I wanted them to remain separate tests.

If you still think that is a bad idea, I could just make this update do callPackage, and not move the other tests, or refactor the files to just return strings and have it construct 1 drv out of them? But I kinda like being able to run those on their own?

@BirdeeHub
Copy link
Copy Markdown
Owner Author

BirdeeHub commented Apr 15, 2026

If you wanted to add some utilities, you would pass them here, alongside self.

result = pkgs.callPackage value { inherit self; };

But the utilities would be for setup/util/multiple cases within a particular test, like you have them in the direnv one more or less, and maybe have a helper for calling a dir and returning a set of tests from it like I did in the makeWrapper and symlinkScript check.nix files. Oh, and an easier way to disable a test for the relevant systems

So, for the direnv one, you would probably still do it the way you are doing it now, this just gives the option for multiple if you want to actually be able to run them separately from one another, and calls them with callPackage which is nicer (lets you grab lib and runCommand and stuff directly)

Honestly, the motivation for doing this was, I had many files with many drvs, which I could run separately, and I wanted to move them to somewhere where I couldn't have that and this was easier than refactoring them individually.

Honestly I wish we could group them and still run them separately too, but flakes don't allow that at the moment.

So, the idea would be, have helpers so that as few people reach for this as possible, but still allow it, and maybe one day we get to group them.

@BirdeeHub BirdeeHub force-pushed the tests branch 3 times, most recently from c679efa to 858ebf0 Compare April 15, 2026 06:57
…dule

also allowed module check.nix files to return a set of packages if
desired, instead of just 1 package or null
@zenoli
Copy link
Copy Markdown
Contributor

zenoli commented Apr 15, 2026

nix build ./ci#checks.<system>.wrapperModule-thatone will only run the checks if it provides a single derivation

Otherwise if you were returning a set it would be

nix build ./ci#checks.<system>.wrapperModule-thatone-attrname

But, theoretically if you had multiple for your module, that would be what you want, and otherwise you wouldn't care?

Ok. This seems fine for me.

It is something that should mostly be used by helper modules to be honest. The nix command + flake schema doesn't really give us a good way to group tests.

Yes, I think it makes sense to mostly stick to one test for wrappers.

But I wanted to move the tests for the helper modules to the helper modules, and I didn't want to group them all into 1 big derivation I wanted them to remain separate tests.

If you still think that is a bad idea, I could just make this update do callPackage, and not move the other tests, or refactor the files to just return strings and have it construct 1 drv out of them? But I kinda like being able to run those on their own?

No, from what I've read this sounds perfectly reasonable.

@BirdeeHub BirdeeHub merged commit 252e9b5 into main Apr 15, 2026
2 checks passed
@BirdeeHub BirdeeHub deleted the tests branch April 15, 2026 16:35
@BirdeeHub
Copy link
Copy Markdown
Owner Author

Gonna just go with it then.

Theyre tests, so not part of the public API other than for modules being submitted here, so, the risks associated with changing stuff are not a thing (i.e. people coming here and being mad in the issues section for changing stuff without warnings)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants