fix #15631

[n5m]

fix #15631

[Clyybber]

The tests without -d:useMalloc should be removed as they only really test that we don't use malloc or that valgrind can only detect leaks via malloc (not something we should test or rely on); we also don't need seperate tests for orc/arc in this case IMO.

[n5m]

What is the best way to get the Valgrind tests to pass on non-Linux hosts in the pipelines?

One way could be to rewrite the tests to look like

discard """
   # Testament configs
   # Testament configs
   # Testament configs
"""
when defined(linux) and sizeof(int) == 8:
  # original test code
  discard alloc(1)
else:
  # simulate the expected failed valgrind output
  echo "   definitely lost: 7 bytes in 2 blocks"
  # exit with the same code as --error-exitcode=1
  quit(1)

which follows the example set here:

Nim/testament/specs.nim

Line 308 in d6160ef

when defined(linux) and sizeof(int) == 8:

and fakes the output expected in test cases like

Nim/tests/valgrind/tleak_arc_useMalloc.nim

Line 5 in d6160ef

outputsub: " definitely lost: 7 bytes in 2 blocks"

but this does not seem like a great solution.

[n5m]

Oh, this looks like it will be much better:

Nim/testament/specs.nim

Lines 317 to 325 in d6160ef

	of "disabled":
	case e.value.normalize
	of "y", "yes", "true", "1", "on": result.err = reDisabled
	of "n", "no", "false", "0", "off": discard
	of "win", "windows":
	when defined(windows): result.err = reDisabled
	of "linux":
	when defined(linux): result.err = reDisabled
	of "bsd":

Nim/testament/specs.nim

Lines 339 to 341 in d6160ef

	of "32bit":
	if sizeof(int) == 4:
	result.err = reDisabled

[Clyybber]

tests/arc/tasyncorc.nim and tests/arc/thavlak_orc_stress.nim leak :)

[Clyybber]

Sorry for force-pushing to your branch @n5m , I thought that a rebase would solve the errors.
I now added a leaks switch to the valgrind option to accomodate for the two failing tests.
(Test failures unrelated)

[n5m]

Quite all right, @Clyybber. Happy to have helped with this PR.

I am interested, however, in seeing tvalgrind.nim added back into the codebase. My rationale is that while tleak_arc.nim tests that memory leaks are found, tvalgrind.nim tests Testament, itself. It also nicely parallels the remaining tests in testament/tests/shouldfail/, which test Testament's other configuration options.

Would you be opposed to a tvalgrind.nim like this?

discard """
valgrind: enabled
cmd: "nim $target --gc:arc -d:useMalloc $options $file"
disabled: "freebsd"
disabled: "macosx"
disabled: "openbsd"
disabled: "windows"
disabled: "32bit"
"""

when defined(linux) and sizeof(int) == 8:
  # test that a memory leak is caught by valgrind, but valgrind only
  # runs on 64-bit Linux machines...
  discard alloc(1)
else:
  # ...so on all other OS/architectures, simulate the same exit code
  # that valgrind would have thrown for this test case
  quit(1) # chosen from --error-exit=1

[Clyybber]

@n5m Oh, yeah that looks fine.