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.

Sign up for GitHub

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

Jump to bottom

tests.references: use runNixOSTest instead of nixosTest #292759

Merged
merged 1 commit into from
Jun 20, 2024
Merged

tests.references: use runNixOSTest instead of nixosTest #292759

merged 1 commit into from
Jun 20, 2024

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Mar 2, 2024 •
edited
Loading

Description of changes

As #282401 makes NixOS tests available on MacOS, it is now possible to run NixOS tests on all the platforms OfBorg would test against. Packages tests built with testers.runNixOSTest should now benefit from such cross-platform support. Only NixOS test running on MacOS is brought by #282401 so far. See #294725 (comment) for detail.

This PR migrates tsets.references from nixosTest to runNixOSTests and makes it available on all platforms. See also #293891.

See also #273183 for exposing test.references.testScriptBin on non-Linux platforms.

Cc: @roberth

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 2, 2024
@ShamrockLee
Copy link
Contributor Author

@ofborg build tests.trivial-builders.references

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 6, 2024 •
edited
Loading

@ofborg build tests.trivial-builders.references

@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 6, 2024
@ShamrockLee ShamrockLee changed the title tests.references: make available on Darwin tests.references: use runNixOSTest and make available on Darwin Mar 10, 2024
@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Mar 10, 2024 •
edited
Loading

@ofborg build tests.testers.runNixOSTest-example

@ShamrockLee ShamrockLee mentioned this pull request Mar 10, 2024
Open
@ShamrockLee ShamrockLee changed the title tests.references: use runNixOSTest and make available on Darwin tests.references: use runNixOSTest instead of nixosTest Mar 10, 2024
@ShamrockLee ShamrockLee marked this pull request as ready for review March 10, 2024 18:35
roberth
roberth reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Have two suggestions for a solution. One is closer to the current solution; the other, packages.nix is a bit cleaner.

pkgs/build-support/trivial-builders/test/references/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/trivial-builders/test/references/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/trivial-builders/test/references/default.nix Outdated Show resolved Hide resolved
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Mar 15, 2024
@ShamrockLee ShamrockLee force-pushed the test-references-darwin branch 2 times, most recently from fa1018f to 56a643e Compare March 15, 2024 23:48
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes and removed 10.rebuild-linux: 1 labels Mar 16, 2024
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog labels Mar 16, 2024
@ShamrockLee
Copy link
Contributor Author

I just hard-code the attribute path (tests.trivial-builders.references.testScriptBin). Changing attribute name / attribute path itself is expected to involve manual fixes. Let's keep it simple.

@ShamrockLee
tests.references: migrate from nixosTest to runNixOSTest
abf7172 
- Use testers.runNixOSTest instead of testers.nixosTest as nixosTest
  has become obsolete.

- Prepare for cross-platform testing.
    - Use the testScriptBin passthru'd by the references test package
      inside the guest pkgs.
roberth
roberth approved these changes Mar 17, 2024
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Maybe my suggestion was more complicated than needed.
It looks pretty good now!

@roberth
Copy link
Member

roberth commented Mar 17, 2024 •
edited
Loading

@ofborg build tests.trivial-builders.references

@ShamrockLee
Copy link
Contributor Author

Maybe my suggestion was more complicated than needed.
It looks pretty good now!

Your suggestion was valid, as I initially wanted to make the attribute path relocatable/overridable, and your suggestion was a cleaner approach to achieve it.

Later I found that such desire not only complicate the implementation but also compromise overridabilty of the test case itself, so here it is.

@ShamrockLee
Copy link
Contributor Author

@roberth #273183 proposes a way to forward the interface of this test case onto non-Linux platforms. I turned that PR to draft to wait for #178717.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 19, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1756

@roberth roberth merged commit cb5a028 into NixOS:master Jun 20, 2024
29 checks passed
@roberth
Copy link
Member

roberth commented Jun 20, 2024

Thanks!

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

@roberth roberth roberth approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ShamrockLee @roberth @nixos-discourse @wegank