tests: fail on warnings and minor refactor#1986
Closed
MattSturgeon wants to merge 1 commit intonix-community:mainfrom
Closed
tests: fail on warnings and minor refactor#1986MattSturgeon wants to merge 1 commit intonix-community:mainfrom
MattSturgeon wants to merge 1 commit intonix-community:mainfrom
Conversation
MattSturgeon
force-pushed
the
test_module
branch
from
78e3542 to
9382186
Compare
MattSturgeon
commented
Aug 4, 2024
tests/test-derivation.nix
Outdated
| module | ||
| ./_module.nix | ||
| ../modules/top-level | ||
| ] ++ lib.optional (args ? dontRun) { tests.dontRun = lib.mkDefault dontRun; }; |
Member
Author
There was a problem hiding this comment.
It'd be simpler to drop the dontTest attr in moduleList. If we keep the function arg/attr then we need to pass it in to the module config here using mkDefault.
@traxys is dropping the attr/fn-arg a breaking change? I assume yes, but probably not an important one?
It'd be nice to have dontRun only be a module option (other than in mkTestDerivationFromNvim)
MattSturgeon
commented
Aug 4, 2024
tests/test-derivation.nix
Outdated
Comment on lines
+110
to
+127
| errors = pkgs.runCommand name { inherit name assertions warnings; } '' | ||
| echo "Issues found evaluating $name": | ||
| if [ -n "$assertions" ]; then | ||
| echo "Unexpected assertions:" | ||
| for it in "$assertions"; do | ||
| echo "- $it" | ||
| done | ||
| echo | ||
| fi | ||
| if [ -n "$warnings" ]; then | ||
| echo "Unexpected warnings:" | ||
| for it in "$warnings"; do | ||
| echo "- $it" | ||
| done | ||
| echo | ||
| fi | ||
| exit 1 | ||
| ''; |
Member
Author
There was a problem hiding this comment.
Should this be done on a per-module basis or on a per moduleList basis?
If the latter, how should we handle some modules having errors and others needing to be build/run?
5 tasks
6 tasks
Assertions failed previously, but now they are handled within a test-derivation. Warnings were printed previously, now they will fail the test.
MattSturgeon
force-pushed
the
test_module
branch
from
9382186 to
01ba842
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dontRuna config optionThis PR refactors things so that warnings and assertions are explicitly checked by the test functions.
The biggest benefit is that it will now be more obvious when tests produce warnings.
Additional benefits include:
tests.dontRunis now a module option, simplifying the implementationIn the future we can add additional
tests.*options, such as "expect" options, assertion/warning whitelists, etc.Marked as a draft because I'm too tired to test this and ensure it is in a good working state, but code reviews are still welcome!