Replies: 16 comments 23 replies
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
@alaviss is your mapping like this:
I emphasized my interpretation of your mapping. |
Beta Was this translation helpful? Give feedback.
-
|
During discussion, I think |
Beta Was this translation helpful? Give feedback.
-
|
Updated diagram - "disabled" stays the same, "known issue" is for things like "make CI green again", people filing bugs, is finding specification does not work and so on. |
Beta Was this translation helpful? Give feedback.
-
|
Created this issue in order to track the We can discuss the design, if required there. |
Beta Was this translation helpful? Give feedback.
-
|
List of quality of life improvements that were discussed in addition to the
And other things
|
Beta Was this translation helpful? Give feedback.
-
We can start by implementing a parser for megatest's output, then silence successful tests. |
Beta Was this translation helpful? Give feedback.
-
|
#96 fixes until we rehaul testament |
Beta Was this translation helpful? Give feedback.
-
|
One thing we might want to decide is whether to keep the test spec in the test file itself. Currently this does come with the caveats of causing line-shift, so maybe we might want "smarter" test spec that can adapt. ie. special comments: ##! err(col = 9): Error: x is undefined
var y = x |
Beta Was this translation helpful? Give feedback.
-
|
I prefer in file, I'd rather invest in special handling and what not instead. So plus one to special comments or the like. |
Beta Was this translation helpful? Give feedback.
-
|
I think it is not possible to properly test this error message, but it must also be supported t03_tuple_borrow.nim(16, 7) Error: cannot borrow view; what it borrows from is potentially mutated
t03_tuple_borrow.nim(19, 9) the mutation is hereI need to check line location for both messages |
Beta Was this translation helpful? Give feedback.
-
|
Maybe we should just fix that message, sorta sucks. |
Beta Was this translation helpful? Give feedback.
-
|
This is related to the testing - right now it is very hard to check compiler output in a structured way, because all it does is print out a stream of loosely formatted text that cannot be reasoned about. There are hacks like https://doc.rust-lang.org/rustc/json.html https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Diagnostic-Message-Formatting-Options.html json diagnostics I think this is an important part of the compiler testing - we no longer would have to edit unit tests on each phrasing change (and considering I want to address horrendous qualify of the compilation errors in the future, it would be a very annoying point of friction). |
Beta Was this translation helpful? Give feedback.
-
|
Yes, I don't think it is even possible to design a completely stable output format like this from scratch. Eventually it will be stabilized of course. Ideally I would like to have a |
Beta Was this translation helpful? Give feedback.
-
|
After some progress on the structured report PR #94 I can say this idea is good enough. Probably no need to support special syntax in the josn checks - instead we can do a line-by-line structural diff of the compiler output and expected json text. Test does not have to specify all the fields unless this is necessary of course. I think, in order to make tests easier to edit we can output things in S-expressions format, we have a parser for it https://nim-lang.org/docs/sexp.html already in the compiler, so we just need to plug it in. Compiler can output S-expression lines as well, and there is absolutely nothing different from json except for the lack of quotes and generally improved ergonomics. Also would make it much easier to do a detailed check of the |
Beta Was this translation helpful? Give feedback.
-
discard """
nimout: '''
{"category": "Sem", "semReport": {"kind": "ExpandMacro", "expression": <AST of the new>, "original": <AST of the new> }}
'''
"""{"category": "Sem", "semReport": {"kind": "ExpandMacro", "expression": <AST of the new>, "original": <AST of the new> }}(Sem:ExpandMacro :expression (...) :original (...))Current nim s-expression parser does not handle |
Beta Was this translation helpful? Give feedback.
-
|
Ouch, good finds. I think the s-expressions stuff will be a bigger deal than anticipated. Writing this as a reminder to create a clean issue that might be good as a starter. |
Beta Was this translation helpful? Give feedback.
-
|
Macro/template/TRM expansion tracer (before/after) should benefit from that as well (since S-expressions are made to represent AST in the first place) - with json we would have some wacky hacks like |
Beta Was this translation helpful? Give feedback.
-
|
Part of the discussion that @haxscramper and I had also covered the layer of tests, and though it's not testament related directly, knowing what we're trying to support might be helpful. A couple notes:
|
Beta Was this translation helpful? Give feedback.
-
|
I think it also makes sense to add proper |
Beta Was this translation helpful? Give feedback.
-
|
We should also make testament output all binaries in a specific directory, to ease source management by separating artifacts from source. Better yet, it should do what @disruptek's balls does of generating binary name based on their compilation flags, which would allow tools like |
Beta Was this translation helpful? Give feedback.
-
|
Yes, that would be a nice addition, at the start it drove me nuts that testament thinks it is appropriate to leave tons of random files all over the test directory, and cannot even clean up after itself. |
Beta Was this translation helpful? Give feedback.
-
|
FYI: |
Beta Was this translation helpful? Give feedback.
-
|
Testament should print all test failures after the entire suite is done so that developers can see what has failed without doing searches in their terminal, replacing |
Beta Was this translation helpful? Give feedback.
-
|
#96 caches all the failures into one file in JSON output |
Beta Was this translation helpful? Give feedback.
-
|
Nim's javascript backend is very useful, we already have a number of browser specific modules, these modules have to be tested manually at the moment, which means no CI. After Testament is fixed, we should make a test runner for the The runner could work by executing compiled JS in the browser using a WebDriver API, such as selenium or webdriver.io, and output errors from the browser console.
|
Beta Was this translation helpful? Give feedback.
-
|
Some observations while reworking testament and I couldn't sleep 😅:
I've been bumping my head into this for the past few days and only now is it becoming clearer. We have two very different systems under test. The former compiles (or rejects) builds of the compiler executable (system under test) while the latter is ensuring two major things, 1) the right program was built, and 2) the standard library works as it should. So we have (numbers for reference, not order):
Anyhow, I think this is why testament is so muddled today, it's doing many things and the data model doesn't clearly describe this. It just pretends it's all the same testing and it really isn't. For example 3 and 4 should not care about whether we used a debug/release compiler or which memory model it went with. While the compiled program will care about this in the test matrix |
Beta Was this translation helpful? Give feedback.
-
|
From what I can see the |
Beta Was this translation helpful? Give feedback.
-
|
Some more UX improvement ideas Make testament use all of this is unnecessary noise that can be removed
|
Beta Was this translation helpful? Give feedback.
-
|
I think I have a specific todo list for testament that I want to implement now. After working with it for some time ... basically it is a pure garbage, so I want to at least clean up the implementation a little, no big structural reworking wrt. to how it executes tests and so on.
proc addResult(
r: var TResults,
test: TTest,
target: TTarget,
expected, given: string,
successOrig: TResultEnum,
allowFailure = false,
givenSpec: ptr TSpec = nil,
) =
# instead of `ptr tspec` we could also use `option[tspec]`; passing
# `givenspec` makes it easier to get what we need instead of having to
# pass individual fields, or abusing existing ones like expected vs
# given. test.name is easier to find than test.name.extractfilename a bit
# hacky but simple and works with tests/testament/tshould_not_work.nimShould be replaced with This does not add/remove any new/old features for how testament works, just cleans up the data flow for all of this the tl;dr version is
tl;dr on less important implementation details:
And this will enable us to:
|
Beta Was this translation helpful? Give feedback.
-
|
Was first posted in 1, 2, 3: I agree. 4: maybe, I don't remember 5: yup 6: there are likely gotchas all over this, I suspect there will be all sorts of hidden abuse that makes this hard, tackle this week towards the end and assume you might defer it 7: yes, less gotchas than 6, I would do it before 6, but after the rest. I would put the above in discussion, then let's lift it into a project of one doesn't exist. |
Beta Was this translation helpful? Give feedback.
-
|
Some existing todo issues for this |
Beta Was this translation helpful? Give feedback.
-
|
Added a project for this thread's ideas, it is already pretty focused on task, so things can be organized further https://github.com/orgs/nim-works/projects/4 |
Beta Was this translation helpful? Give feedback.
-
|
When used with |
Beta Was this translation helpful? Give feedback.
-
This discussion is a collection of ideas that primarily @haxscramper, @alaviss, and I have discussed around improvements to testament.
That would be a significant quality of life improvement, either in the short term or in the longer term.
Beta Was this translation helpful? Give feedback.
All reactions