Added knownIssue as key for testament spec

[canelhasmateus]

This references issue 61

• added 'knownIssue' as key for spec.
• 'disabled' is now interpreted as a semicolon separated list of (platform names or truthy values).

KnowIssues is allowed to be mentioned multiple times;
The proposal of no longer accepting boolean values seems to be more intricate, since various tests rely on this option.
Still not handling the case of knownIssue for specific platforms.

[haxscramper]

Thanks for working on this one. Aside from things I commented on, it looks good to me.

[haxscramper]

A minor note: please use human-readable PR and issue names, - issues/61 is hard to make sense of, and your commit has a much better-fit description added 'knownissue' as key for spec.. The same applies to the commit message titles.

[haxscramper]

A known issue for a specific platform is a tricky problem, because fundamentally, it can be seen as a specific entry (or set of entries) in the build matrix - os, gc, backend and so on. So "disable" and "matrix", "target" provide a specific set of options that narrow down the run matrix entry, and then we mark the set of resulting runs as problematic.

I'm not sure if it really makes sense, but for issue like nim-lang/Nim#17552 expected test would look like

discard """
knownIssue: "https://github.com/nim-lang/Nim/issues/17552"
matrix: "--gc:orc"

If all runs of a known issue pass, it means that issue is fully solved, but individual successful runs should also be noted.

At the same time, we have known issues that are not related to specific matrix configuration (general compilation failure like "Accepts invalid", or "rejects valid"), and those are marked as knownIssues without any matrix configurations.

[haxscramper]

So to summarize what I wrote above - I'm not sure if there is even a "case of knownIssue for specific platforms." that need to be handled explicitly. Of course if you had an idea what this should be I would be happy to discuss.

[canelhasmateus]

There you go.
The syntax for disabled is no longer changed,
and the other tests, which were disabled : true
now contain a disabled entry for each platform.

[haxscramper]

changed boolean 'disabled' keys to reference all platform names.

No, disabled: true earlier meant that test is disabled because it is totally broken and/or a known issue. Moving these to reference all platform names is not correct, since it converts the test from "needed to track progress at addressing existing issues" to "disabled on certain platforms due to implementation reasons".

Ideally disabled: true should've been transitioned to the issue link, or textual description of the issue, or knownIssue: true at the very least if it is not possible, or hard to track down a specific reason for the test to be disabled.

[saem]

changed boolean 'disabled' keys to reference all platform names.

No, disabled: true earlier meant that test is disabled because it is totally broken and/or a known issue. Moving these to reference all platform names is not correct, since it converts the test from "needed to track progress at addressing existing issues" to "disabled on certain platforms due to implementation reasons".

Ideally disabled: true should've been transitioned to the issue link, or textual description of the issue, or knownIssue: true at the very least if it is not possible, or hard to track down a specific reason for the test to be disabled.

Have to agree after reviewing, I see two options:

1. Keep disabled true as allowed for now to reduce the number of files changed
2. Convert all disabled true to known issues, but also delete tests that cause problems, eg: there is a test in there mentioning deadlock issues

Even though it's less clean in the long run, I think options 1 will be an easier transition. 😕

[canelhasmateus]

i'm down for the second one.
I believe i'll be able to find the issue for most of these, and will post a update.
We can them discuss what to do about the rest, if any.

[saem]

i'm down for the second one. I believe i'll be able to find the issue for most of these, and will post a update. We can them discuss what to do about the rest, if any.

If you can do that, awesome!

Otherwise, this would put us in a good logical progression point:

• getting known issue in (done)
• converting the tests that should be know issue instead of disabled that we've noted so far (in comments)
• leave disabled bool in for now (revert that bit of the change)
• don't touch tests we don't understand (revert those few)

[canelhasmateus]

Conducted a more in-depth investigation for every test that had a "disabled: true".
Instead of only the ones i didn't find knownIssues for, i decided to jolt down a brief context and the action being done for each one of them, so that we can decide the best course of actions.
Dont mind the formatting, will probably edit it later, if need be.

Mind that some of the issues referenced below are not actually the reason the tests were disabled,
but issues that the tests is trying to fix / prevent regression.
This pattern seems common enough to warrant reification.

assign

\toverload_asgn2.nim 
	Disabled just for CI's sake around 2018-04. 
	at the timeline, very near fixes to issues #7637, #6393, altough doesn't seem related to any of them.
	This test doesn't work, but seems fixable. 
	Running it gives the log:
	Expected:
		i value 88
		2aa

		Gotten:
		i value 88
		0aa

	Action: Keeping `disabled: true`

concepts

\t3330.nim

	Disabled due to nkError issue.  Due to lack of original issue , i'm using https://github.com/nim-works/nimskull/pull/27.
	Before this, it seemed to be disabled in x32 platforms around 2019-07.
	Last time it was fully functional was around 2019-07.
	First time it was disabled was aroud 2018-10.

	Action: added knownIssue

\texplain.nim

	Disabled due to nkError issue.  Due to lack of original issue , i'm using https://github.com/nim-works/nimskull/pull/27.
	First disabled around 2019-02, only at 32bit platforms, due to flaky message ordering

	Action: added knownIssue

\twsizzle.nim
	Disabled due to https://github.com/nim-lang/Nim/issues/2346 , around 2015-03
	not sure what this is actually supposed to test.

	Action: keeping `disabled: true`, but looks like a fine removal candidate. 

\twrapconcept.nim
	Disabled due to nkError issue.  Due to lack of original issue , i'm using https://github.com/nim-works/nimskull/pull/27.
	References https://github.com/nim-lang/Nim/issues/5127

	Action: added knownIssue

cpp

\tdont_init_instantiation.nim

	References  https://github.com/nim-lang/Nim/issues/5140
	Disabled around 2016-12, for CI sake, it seems.


	Action: keeping disabled true.

\tterminate_handler.nim
	Disabled in 2020-03 , in a commit that references 13695
	References 10765

	Action: added knownIssue 13695

\tvectorseq.nim
	created disabled, around 2015-04, in a commit that references 2536

	running it results in the following log:
	Expected:
		(x: 1.0)
		(x: 0.0)

		Gotten:
		(x: 1.0)
		(x: 1.0)

	Action: keeping disabled true.

dll

\nimhcr_integration.nim

	created around 2019-02 in a commit that references 10729
	disabled on macosx around 2019-10 due to flakiness
	disabled on openbsd around 2020-04 in a commit that references 12105
	disabled on netbsd  around 2020-10 in a commit that references 15458
	disabled fully around 2021-03 in a commit that references 17311

	Action: 

effects

\toutparams.nim
	created disabled around 2020-06 in a commit that references 14521

	the only error in this test is the variable name - possibly an easy fix.
	Expected:
	use explicit initialization of 'x' for clarity [Uninit]

	Gotten:
	use explicit initialization of 'tmp' for clarity [Uninit]

errmsgs

\tundeclaredfield.nim
	Disabled due to nkError issue.  Due to lack of original issue , i'm using https://github.com/nim-works/nimskull/pull/27.

	Action: Added knownIssue

compiler

\tdebugutils.nim 
	was born disabled. I guess it is used only for demonstration purposes? 
	from my current understanding, these type of tests should be coalesced into the specs?

fragmentation

\tfragment_alloc.nim and \fragmentation\tfragment_gc.nim
	Seemed to always present flakiness in their lifetimes, being changed several times,  referencing #8509, #7120, #8510 
	Disabled on windows around 2018-03
	Disabled fully around 2018-10, in a commit that references #9421
	
	These two actually passed sucessfully in my pc ( Windows) , but i guess that it is flaky. 
	
	Action: added issues. Maybe re-enable if shown to be not-flaky.

generics

\toverloading_typedesc.nim

	Crreated around 2015-06 in a commit that references #3022
	Disabled around 2017-11 for CI sake?
	Working in my pc.

	Action: Keeping disabled: true . Maybe re-enable.

global

\tglobal.nim
	Disabled around 2015-03, for CI sake?
	Working on my pc.
	Action: Keeping disabled: true.

iter

\tchainediterators.nim
	Disabled around 2015-01 in a commit that references #1838
	Action: Added known issue.

\titerable.nim
	Created in 2014-03 in a commit that references #966
	Disabled around 2015-11 citing 'lambda lifting support for iterToProc plugin'		
	Action: keeping disabled: true
\tscheduler.nim
	Disabled around 2015-07, citing dubiousness ( flakiness? )
	Working in my pc.
	Action: keeping disabled: true

macros

\tmacro7.nim
	Created around 2019-05 in a commit that references #7792
	Disabled around 2019-08 in a commit that references #11883
	Also, references #7120 and #4547

	Action: Added knownIssue. But probably delete, since it seems to use no-longer-allowed behaviour.

method

\tmultim.nim and method\tmultimjs.nim 
	Disabled iaround 2020-03 in a commit that references #13803, for CI sake?
	Working in my pc.
	Action: keeping disabled

misc

\tdlvar.nim
	Created around 2014-04 in a commit that references #1075
	Disabled around 2018-11 for CI sake?
	Action keeping disabled.

\tgenconstraints.nim
	Created disabled around 2014-04 in a commit that references #1075
	Running gives the output:
		Expected:
		cannot instantiate T2

		Gotten:
		undeclared identifier: 'TNumber'
	Action: keeping disabled. Possibly fixable.
	
\tmissingnilcheck.nim
	Created disabled around 2014-04 in a commit that references #1075

\tnoinst.nim
	Created around 2014-04 in a commit that references #1075
	Disabled around 2014-12 in a commit that references #1708, #871
	Running gives the output:
		Expected:
		instantiate 'notConcrete' explicitly

		Gotten:
		internal error: proc has no result symbol

 \tunsignedcomp.nim
 	Created around 2015-01
 	Disabled around 2015-01, not sure why.
 	Action: keeping disabled.

modules

\tnotuniquename2.nim
	Disabled around 2019-04 in a commit that references #11064
	Seems to be testing removed behaviour.

	Action: keeping disabled. Probably should be removed.

\trecmod.nim
	not sure?

navigator

\tincludefile.nim
	Created around 2021-04 in a commit that references #17784
	Disabled around 2021-07 due to flakiness.

	Action keeping disabled. Probably should be removed.

objects

\tobjpragma.nim
	Disabled around 2015-03 for CI sake
	There seems to be a ton of issues referencing this:
	https://github.com/nim-lang/Nim/issues/15752
	https://github.com/nim-lang/Nim/issues/9668
	https://github.com/nim-lang/Nim/issues/10082
	https://github.com/nim-lang/Nim/pull/10227
	https://github.com/nim-lang/Nim/issues/4763

	seems to have been fixed on https://github.com/nim-lang/Nim/pull/10227
	
	working on my pc

	Action: keeping disabled. Probably re-enable

objvariant

\tfloatarangeobj.nim
	Created around 2019-05 in a commit that references #11264
	Disabled around 2019-11 in a commit that references #12379 , #12591
	Action: Added knownIssue

parallel

\treadafterwrite.nim
	Created disabled around 2014-12 in a commit that references #1597
	Action: keeping disabled.


\t5000.nim
	Created around 2014-11 in a commit that references #1646
	Disabled around 2014-12.
	Working on my pc, but probably flaky
	what is this even testing, though
	Action: deleted

pragmas

\tpragmas_misc.nim

	Created around 2020-05 in a commit that references #14443
	Changed in a commit that references #15920
	Disabled around 2021-10 for CI sake

	Action: keeping disabled.

proc

\t17157.nim
	Created around 2021-03 in a commit that references #17157 and #17531
	Disabled in a commit that references #18113 and #18124
	Action: Added known issues.

stdlib

\tparsopt.nim

	Created around 2008.
	Disabled around 2018-11 for CI sake
	Action: Keeping disabled. Probably delete.

system

\t7894.nim
	        Created around 2018-08 in a commit that references #7894 and #8496
	        Disabled around 2019-02 for CI sake - references #10608
	        Seems to run fine
	        Action: keeping disabled.

\tio.nim
	Disabled around 2011-12 because of infinite blocking, reenabled shortly after
	Disabled around 2018-11 due to 'just causing problems'
	Action: keeping disabled.

template

\tmodulealias.nim
	Disabled around 2011-05 in a commit that references #30 and #26.		
	Action: keeping disabled. Probably delete.

typerel

\tno_gcmem_in_shared.nim
	Disabled around  2014, back when 'shared' was a keyword.
	Action: keeping disabled. Probably delete.

\trectuple.nim
	Disabled around 2014 in a commit that 'tuple field names are ignored'
	Action: keeping disabled. Probably delete.

\trectype.nim
	Disabled around 2016-12.
	This seems to correctly validate errors in recursive types, just that the message changed over time.
	Running it gives:
		Expected:
		internal error: cannot generate C type for: PA

		Gotten:
		illegal recursion in type 'PA'

	Action: keeping disabled. Probably fixable.

types

\tisop.nim
	Disabled around 2014 in a commit that 'tuple field names are ignored'
	Action: keeping disabled. Probably delete.

[saem]

why the #?

[canelhasmateus]

For now, i'm keeping the ability to use boolean values in the disabled key.
I changed the disabled to reference an issue, for tests that i could directly tie the disabling commit to an issue.
Mind you that i did not validate that the issue is correct.

I'll be opening a second PR for the remaining 'boolean disabled' tests, so that we can deal with them.

[saem]

Heck yeah!

Please rebase the commits and include your excellent analysis on a test by test basis as part of it.

[saem]

@haxscramper I'm g2g for this PR, how ya feeling about it?

[canelhasmateus]

re-structured the commit message. This should be good to go.

[haxscramper]

It looks good to me, I think I will PR follow-up documentation update to reiterate on the explanations from this PR, and some other thoughts that I have (from #59 where I started creating new tests with knownIssue)

[saem]

Bors r+

[bors]

Build succeeded:

• All check passed