Nim icon indicating copy to clipboard operation
Nim copied to clipboard

[Testament] Extend and document message testing aids

Open quantimnot opened this issue 2 years ago • 2 comments

Inline Messages

After discovering the inline messages feature of testament, I realized how it would remove annoyances I've had testing messages. The inline error messages are very useful when building up a suite of tests, because you don't have to keep changing the line location as your tests get changed around. However, inline error messages don't support anything other than error messages or when the test action is reject. That behavior doesn't allow testing desired hints and warnings with a test action of compile or run.

I extended the inline message feature by:

  • Supporting actions other than reject. This affects these current tests:

    • Inline message not checked because the action is not reject: https://github.com/nim-lang/Nim/blob/8fb4d7ebed81447e0d787846062cf648e50a3158/tests/pragmas/tvar_macro.nim#L87-L89

    • https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/tests/strictnotnil/tnilcheck.nim#L1-L32

  • Supporting more than one message for a line. I'm making use of this in #20095: https://github.com/nim-lang/Nim/blob/f7cfcfe32057e343e44d2c1791f915a6a878b318/tests/stylecheck/thint.nim#L23-L26 This change is enabled by:

    • Adding a message delimiter re";\s*tt\." (regex not actually used).

    • Changing the message line determination from the caret's (^) line - 1, to that of the opening inline message pattern #[tt..

  • Supporting using it along with nimout. This is useful for simultaneously testing inline messages and expected messages from imported modules. I make use of this in #19925 where I'm testing inline messages, messages raised in a nimscript, and messages raised in another module: https://github.com/nim-lang/Nim/blob/1df06003ee588b3e2d12e68b4314902d585dbf7a/tests/modules/tpatchModule.nim#L1-L23

  • Fixed inline message parsing bug that broke multi-line messages. The bug is that newlines are dropped from inline messages: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/specs.nim#L162-L165 There is an existing test that is affected by this: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/tests/effects/teffects1.nim#L1-L44

    What actually happens in this case is:

    • The test spec and inline messages are parsed in testament/specs: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/specs.nim#L181

    • The test action is internally set to reject, because at least one of the inline messages is an Error message: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/specs.nim#L172

    • nimout is ignored because the test action is reject and there are inline messages: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/testament.nim#L545-L547 https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/testament.nim#L390-L397

    • Compiler messages are compared with inline messages by checking if the actual message contains the inline message instead of if they are identical: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/testament/testament.nim#L358 The message of concern in this test is: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/tests/effects/teffects1.nim#L6-L7 The actual message has a newline and .raise effects differ, but the inline message is missing that part: https://github.com/nim-lang/Nim/blob/528b6d1c3f001944a7a7754c8884178e365e6871/tests/effects/teffects1.nim#L38-L42 You can change the inline message to this and it will still pass:

      var p: MyProcType = foo #[tt.Error
                          ^]#
      

Variable substitution for nimout, errormsg and inline messages

Path separator and test filename variable substitution in nimout, errormsg and inline messages. The path separator is needed to handle messages that contain native filesystem paths. I use this in #19925: https://github.com/nim-lang/Nim/blob/1df06003ee588b3e2d12e68b4314902d585dbf7a/tests/modules/tpatchModule.nim#L9

Other Bugs

nimout, ccodecheck and maxcodesize are not handled when output or outputsub are defined. It's useful to be able to write a single test than can:

  • Check a subset of compiler messages in nimout.
  • Check for a output substring with outputsub.
  • Check code gen with ccodecheck and maxcodesize.

The changes for this were done here instead of another PR, because the changes to facilitate using inline messages with actions other than reject also cleanly enabled this.

Breaking Changes

Since this PR changes the behavior of nimout, errormsg and inline messages to be interpolated by means of strutils.%, then the variable marker ($) needs escaped in messages that don't intend any variable substitution.

I think this is a minor breaking change, but a new boolean spec field could be added (ex. subMsg) that would only format those values when it is true.

quantimnot avatar Jul 10 '22 21:07 quantimnot

@ringabout, thank you for letting me know this PR is confusing. I've tried to clarify it in the first comment.

quantimnot avatar Jul 29 '22 21:07 quantimnot

This is ready for review.

Any questions or comments?

quantimnot avatar Aug 01 '22 12:08 quantimnot

Personally, I'd merge this in now, but @Araq will need to give the final approval.

Varriount avatar Aug 31 '22 18:08 Varriount

@Varriount Thank you for taking the time to review it.

quantimnot avatar Sep 01 '22 15:09 quantimnot

Thanks for your hard work on this PR! The lines below are statistics of the Nim compiler built from 6289b002b6a1ac8dcf5a97075f4db550df080191

Hint: mm: orc; threads: on; opt: speed; options: -d:release 163925 lines; 12.197s; 842.105MiB peakmem

github-actions[bot] avatar Sep 01 '22 15:09 github-actions[bot]