Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/nixpkgs: move nixpkgs module test to dedicated drv #2740

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MattSturgeon
Copy link
Member

We don't need a full test that actually builds or runs nixvim. All we need is some assertions on the result of a nixvim configuration.

Currently #2738 needs to change the module tests to construct their own pkgs instance, so that we can test how the module behaves. However this is likely to have a performance hit since we are repeatedly instanciating pkgs again for each test case. It's probably better to use a single pkgs instance for all test case modules, but move the nixpkgs-module tests to their own dedicated test derivation.

This PR does that, moving the existing tests to a dedicated link-farm.

This also allows creating some bespoke helpers to streamline writing the tests and may improve performance slightly because we don't need to also build config.build.package for the nixpkgs-module unit tests.

The bespoke helpers are a little cumbersome, so I'm open to feedback on them. Some of them may also be useful more generally, so may be worth moving to some kinda "test utils" file. This can be done now or later.

We don't need a full test that actually _builds_ or _runs_ nixvim.
All we need is some assertions on the result of a nixvim configuration.
@MattSturgeon MattSturgeon requested a review from a team December 24, 2024 14:33
@MattSturgeon
Copy link
Member Author

FYI #2738 is kinda blocked on this. Or at least blocked on deciding whether we will merge this.

I'd like to re-write the tests in that PR based on this PR.

@traxys
Copy link
Member

traxys commented Dec 27, 2024

I'd be in favour of introducing in this PR the framework for tests that don't launch nvim, it can be useful to add tests for assertions or warnings (#2751 reminded me that I wanted to add a diagnostic & such a test)

@MattSturgeon
Copy link
Member Author

I'd be in favour of introducing in this PR the framework for tests that don't launch nvim, it can be useful to add tests for assertions or warnings (#2751 reminded me that I wanted to add a diagnostic & such a test)

That's still on my radar, but I don't see how it relates to this PR

@traxys
Copy link
Member

traxys commented Dec 27, 2024

Those tests don't require to launch nvim, so I guess what I am mostly saying is that it would be nice to create in this PR a test suite that does not eval nvim, and allows to easily add tests to it

@MattSturgeon
Copy link
Member Author

Ok, now that we've tackled negative tests can we revisit this PR and discuss whether we will merge it or something like it?

While we could set test.buildNixvim = false and keep the nixpkgs module tests in test-sources, that'd require all the test-spurces modules to not define nixpkgs.pkgs.

IDK how performance intensive instantiating an instance of nixpkgs is, or how well nix is able to cache similar instances for re-use...

Given the sheer number of test case modules we have, I feel it's probably a good idea to explicitly use the same pkgs instance for all of them.

If so, that means keeping the nixpkgs module tests separate, as per this PR (or similar).

@traxys
Copy link
Member

traxys commented Dec 27, 2024

Yeah it addresses my comment, so I have no idea about the performance though :/

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