Macro Code Coverage

[Blacksmoke16]

One aspect of code coverage that is not currently handled via kcov and such, is that of compile time code coverage for macros. Being able to know if there are unused code paths within conditional macro logic would be really helpful. Mainly given there wouldn't be an error until it's hit if there was a bug/typo within that branch. It would be great if Crystal could generate some sort of simple coverage file for this to make use of the same tooling. E.g. something along the lines of https://docs.codecov.com/docs/codecov-custom-coverage-format.

Related: https://github.com/crystal-lang/crystal/issues/1157

[Blacksmoke16]

I started on an prototype implementation of this and have something that seems like it's working quite well. However, the exact design of this feature is still slightly up in the air. A primary use case of this feature will be, for me at least, to run against spec files to ensure the test coverage I have against my macro logic is sufficient and not missing any paths.

Unlike the crystal tool unreachable command, which works fine because all runtime code paths would be identified by it. However, when it comes to macros, due to their nature, a similar tool for macro coverage would only identify the happy paths of the flow. Any macro logic that raises a compile time error, as I do via https://forum.crystal-lang.org/t/testing-compile-time-errors/272/5, would be missed.

Because of this we have a few options on how to best enable the generation of the coverage report:

• Just make it a tool like unreachable
  • Pro: Fits in well with the rest of the tools
  • Con: Running the specs AND generating the report would each require a separate command. I.e. the code has to be compiled twice
  • Pro: Able to be used on any Crystal source file
• Add a new global --macro-code-coverage-report option
  • Pro: Allows running and generating of the report in the same compilation
  • Pro: Can be used on any Crystal source file
  • Con: Adds yet another option that won't be heavily used
• Use some sort of ENV var denoting a directory to save the report(s) to
  • Pro: Allows the report to be generated globally for multiple Crystal commands (run/build)
  • Pro: Would natively be inherited within Process.run to handle generating those sub-report files as well
  • Con: Would be a somewhat new pattern compared to the other options
• Combine with unreachable tool
  • Pro: Able to kill two birds with one stone
  • Con: Needs more vetting as we don't want to jump the gun

I'm currently leaning towards the ENV var, just because it seems to fit my use case the best. But def interested in hearing other's use cases, reasoning for another option, or entirely new options.

[straight-shoota]

An important characteristic of macro code is that it's not only determined by other code, but macros are often used to respond to compiler flags. Depending on the build target or explicit configuration flags, you get different control flow in macros. I can't tell exactly how important that is for coverage testing because you could also just build programs with these flags and test the behaviour. The coverage tool is probably more important for really complex macro logic which I suppose tends to be mostly determined by code (macro calls, annotations) and less by external flags.

In order to fully test a piece of macro functionality that responds to flags, you may need to run it with a bunch of different flag combinations. For that use case it should be possible to easily run only macro coverage for little overhead (i.e. without codegen). That may hint towards a separate tool (crystal tool macro-coverage?), but I'm not sure.

[Blacksmoke16]

The coverage tool is probably more important for really complex macro logic which I suppose tends to be mostly determined by code (macro calls, annotations) and less by external flags.

This is pretty much my use case yea. I don't really have any logic based on compiler flags (that is embedded in the macro logic at least). The logic that is behind flags is tested by running the normal specs on different OSs.

For that use case it should be possible to easily run only macro coverage for little overhead (i.e. without codegen)

For this, would it be easy enough to just pass --no-codegen with the ENV var to a crystal run/build and get the same outcome? It also would be fairly easy to support more than one of these options. E.g. both the tool and ENV var support to best support both of these use cases.

[Blacksmoke16]

Another con of the ENV var is there wouldn't be a good way to control what gets included in the report. I.e. the --include and --exclude options of the unreachable command. Maybe it would be enough to hard-code it to src/ and spec/ tho?

Also will need a way to handle terminal nodes, like raise "". Probably check if we're in this "mode", swallow the error, and gracefully exit?

[Blacksmoke16]

Okay after playing with this more, I have another possible solution for the entry point.

I realized that my concern with it being a tool may not be entirely true. Running the tool itself does essentially what Process.run was doing. I.e. building the code w/o codegen. So instead of doubling the CI time, you could replace Process.run with the tool and everything should "just work?"

However to fully support this, the tool would need (working initial thoughts):

• An optional flag to enable codegen
  • Some of my tests enable codegen to run additional runtime assertions based on the state setup by the macro logic. I now realize this is itself a major slowdown in CI, with 5 specs taking 25s on my machine with codegen, while its only 6s without. So I'll have to see if there's a way to not depend upon this so this flag would be un-needed.
  • EDIT2: Yea, think I came up with a way to handle most of this at compile time. Isn't as clean but it'll do the job. Not yet sure if it removes the need for this option, but we shall see.
• An optional flag to control where the report is written on the filesystem, defaulting to STDOUT.
  • A lot of my macro logic is testing the error states, and because I handled exceptions by just swallowing it, that would now imply there wasn't an error so the test would fail. A challenge related to this is currently the tool is just outputting the report JSON to STDOUT. This might conflict with other output, such as the exception message/trace itself. So would make sense to optionally have this written to a file directly as to not interfere with the actual logic.
  • EDIT: Maybe it would be enough to send the report thru a diff location compared to the other output. E.g. STDERR or something? :shrug:
• Support --stdin-filename as to not require creating a real file
  • Should be pretty trivial, as other commands already support this.
• TBD

Does this sound reasonable? Otherwise, where is everyone leaning in regards how to expose this feature?

[Blacksmoke16]

annotation Test; end

@[Test(id: 10)]
record Foo

module MyModule
  macro included
    macro finished
      {% verbatim do %}
        {%
          pp Foo.annotation(Test)[:id] * 10
        %}
      {% end %}
    end
  end
end

class Container
  include MyModule
end

Container.new

It seems we do indeed need to handle VirtualFile to account for macros expanded via hooks. However, in regards to the pp Foo.annotation(Test)[:id] * 10 node:

pp! node,
  f.source,
  location,
  location.macro_location,
  f.macro.location,
  location.expanded_location

node                       # => pp((Foo.annotation(Test))[:id] * 10)
f.source                   # => "    macro finished\n" +
"      \n" +
"        {% pp((Foo.annotation(Test))[:id] * 10) %}\n" +
"      \n" +
"    end\n" +
"  "
location                   # => expanded macro: included:3:12
location.macro_location    # => /home/george/dev/git/crystal/cov.cr:7:3
f.macro.location           # => /home/george/dev/git/crystal/cov.cr:7:3
location.expanded_location # => /home/george/dev/git/crystal/cov.cr:19:3

There doesn't seem to be a direct way to have it point to the actual location of the node, which would be line 11 in this context :/. Line 3 is correct in relation to f.source, but that doesn't really help me here as I need the real file + line number to report...

Any ideas, or is this something that needs to be added to the compiler?