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

Source filtering and disabling tests (to later run them externally) #7870

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

roberth
Copy link
Member

@roberth roberth commented Feb 20, 2023

Motivation

  • Prevent unnecessary rebuilds by including only relevant files. This speeds up the builds of anything that depends on the main build

  • Provide a solid foundation for working on tests that are run in a test environment that is outside the build of nix itself, catching bugs and preventing regressions that can't be detected there.

Context

Additive or negative filtering

The source filtering approach is constructive instead of based on the removal of specific files. This is a best practice because it does not allow newly added files to accidentally cause unnecessary rebuilds. While a ~20 item list doesn't look great, it is hardly worse than the alternative, which is the removal of 8 files and 4 directory. Furthermore, new additions will typically not need inclusion into the main build, as the main build's tooling is already established and most additions of tooling will be for items that aren't part of the main build, such as the python bindings, or the docker image earlier.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

@roberth roberth changed the title Source filtering and support for external tests Source filtering and disabling tests (to later run them externally) Feb 20, 2023
@roberth roberth added contributor-experience Developer experience for Nix contributors with-tests Issues related to testing. PRs with tests have some priority labels Feb 20, 2023
@edolstra
Copy link
Member

Not sure about the source filtering. That seems like a fair amount of complexity for a somewhat uncommon use case. It would be nice when hacking on the VM tests (usually I use nix.package = toDerivation /nix/store/... to avoid rebuilds).

The ability to disable tests looks useful, maybe that could be a separate PR?

@edolstra edolstra closed this Feb 21, 2023
@edolstra edolstra reopened this Feb 21, 2023
@edolstra
Copy link
Member

Sorry, didn't mean to hit "close".

@Ericson2314 Ericson2314 mentioned this pull request Feb 21, 2023
7 tasks
@Ericson2314
Copy link
Member

@edolstra See also #7876. I do think we have some leeway between moving things around and filtering, in that the more we do of the one, the less we need to do of the other. But I don't think there is a sustainable option of "not filtering and not moving around files" --- we need to do something.

@Ericson2314
Copy link
Member

Based on the discussion in #7847 last time, where we mentioned doing filtering as a prelude to moving things around to avoid needing the filtering, I think we definitely want to do this next.

@roberth
Copy link
Member Author

roberth commented Apr 1, 2023

While I want to agree, as any solution to the rebuilds problem would be helpful, I think we did agree on the goal of moving things, in which case this PR is a bit redundant. My interpretation of the meeting is that we minimize the amount of work done during the move, but we do perform the move (soon?).

@Ericson2314
Copy link
Member

Now that we have positive source filtering, should this be rebased?

@github-actions github-actions bot removed the with-tests Issues related to testing. PRs with tests have some priority label Aug 28, 2023
@roberth
Copy link
Member Author

roberth commented Aug 28, 2023

Had to reindent the filter, so here's a link to view the files with whitespace changes hidden.

imports = [
test
# For fast cycle when you don't need the regular tests:
# (useNixWithoutRegularTests system)
Copy link
Member Author

@roberth roberth Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not enabled by default because it does duplicate the compilation work if you want to run both VM and non-VM tests. (I figured I'd be over-explaining by adding that sentence to the code)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants