-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix #16256: nimout: <empty> should give error (vacuously true); improve a few tests #18089
fix #16256: nimout: <empty> should give error (vacuously true); improve a few tests #18089
Conversation
testament/specs.nim
Outdated
@@ -401,6 +403,9 @@ proc parseSpec*(filename: string): TSpec = | |||
if skips.anyIt(it in result.file): | |||
result.err = reDisabled | |||
|
|||
if nimoutFound and result.nimout.len == 0 and not result.nimoutFull: | |||
doAssert false, "empty `nimout` is vacuously true, use `nimoutFull:true` if intentional" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Report spec errors as errors, doAssert
is not a mechanism for reporting errors. Errors are not bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…); improve some tests
be3162a
to
1994128
Compare
I can't comment on the testament changes, but the test changed look good (it's a big improvement!). |
…); improve a few tests (nim-lang#18089) * fix nim-lang#16256: nimout: <empty> should give error (vacuously true); improve some tests * renamed: tests/stdlib/t9710.nim -> tests/misc/t9710.nim * improve tests * fix non-DRY tests * improve $nim_prs_D/tests/stdlib/t9091.nim * renamed: tests/stdlib/t9091.nim -> tests/misc/t9091.nim * fixup * address comment: doAssert => result.parseErrors
nimout: <empty>
should give error (vacuously true) #16256eg:
targets: "c"
is bad (prevents honoring $target which can be cpp in CI via NIM_COMPILE_TO_CPP for eg)action: "compile"
whereas the test should actually runexitcode: 0
oraction: "run"
which are the defaulttimeout: 60.0
which are there for no valid reasonnimout: ""
which is vacuously true (ie testament:nimout: <empty>
should give error (vacuously true) #16256, which i'm fixing in this PR)--styleCheck:hint --panics:on
which have no effect as far as the test is concernedoutput:
withaction: "compile"
) or don't check the resultstos.nim
output: "test AObj"
which should be replaced by doAssert / check(also fixes #13952 (comment), #13952 (comment))
(not that there are exceptions to above rules but they're rare and don't apply in this PR)
future work
testament should give doAssert/spec error when it detects bad combinations instead of silently accepting and succeeding with some arbitrary resolution, eg:
action: "compile"
+output
should errortargets: "c cpp"
+cmd: "nim c ..."
should errorideally, it would also give error under some
--specCheck
testament flag for vacuously true spec flags or suspicious combinations of flags to help users improve their tests