crystal icon indicating copy to clipboard operation
crystal copied to clipboard

Run spec suite in `macro finished` hook instead of `at_exit`

Open straight-shoota opened this issue 1 year ago • 4 comments

The execution of specs currently runs in an at_exit handler. It means the main program itself is basically empty, except for maybe some setup code. And I presume that's the reason why specs run in at_exit because it ensures it runs after any other top-level code, regardless of require order. Conceptually however, the spec runner is the actual main program, yet it only starts when the process is already exiting. This seems to be a bit of a hack and can interfere with other at_exit handlers (https://github.com/crystal-lang/crystal/issues/13763#issuecomment-1812418763).

Ideally, the spec runner should execute after any other top-level code but before any other at_exit handlers.

This seems like a good fit for macro finished, which essentially moves its code block to the end of the main program.

Running the spec suite directly from a macro finished hook should be largely equivalent to at_exit. It would be semantically cleaner and there would be no contention with other actual at_exit handlers.

Of course, in exchange, there will be contention with other macro finished hooks. But I figure that should not be as much hassle as with at_exit. Most uses of macro finished are for defining methods or constants based on other features defined previously and the purpose of the macro finished is to move the concluding code after the code it relates to. But there's usually no global aspect (in constrast to at_exit which affects the entire program). This should typically not interfere with the spec usage at all.

In order to be on the safe side, it would also be possible to nest macro finished, which moves the nested block at the very end even after all "single-level" macro finished.

macro finished
  macro finished
    puts "3"
  end
end

macro finished
  puts "1"
end

macro finished
  puts "2"
end

straight-shoota avatar Dec 20 '23 13:12 straight-shoota

It's hard to say until there's an initial implementation to run, but it's possible this will affect some Athena stuff in that one component is doing something like:

module ModuleOne
  macro included
    macro finished
      {% verbatim do %}
        puts "1"
      {% end %}
    end
  end
end

module ModuleTwo
  macro included
    macro finished
      {% verbatim do %}
        puts "2"
      {% end %}
    end
  end
end

class Bar
  macro finished
    include ModuleOne
    include ModuleTwo
  end
end

Which has the same finished level as what the spec finished would be in. Would there be a use case for being able to do something like macro finished(terminal: true) to force one to always execute last no matter how many nested levels there are?

Blacksmoke16 avatar Dec 20 '23 14:12 Blacksmoke16

I don't expect there to be many issues because the code in the finished hooks probably isn't some actual top-level code like puts "1". Instead, it's likely some def or similar, right? Then there should be no conflict with the top-level code for the spec runner.

The implementation is quite trivial, actually. You can give it a try with #14122 🚀

straight-shoota avatar Dec 20 '23 14:12 straight-shoota

The vast majority of it is all just compile time code that runs in a {% ... %} where the puts is. There's also some code that generates some getters/types within the class the module is included in.

Either way, I built that branch and ran the specs and everything passed so 🤷. I'll keep an eye on nightly CI as well just in case.

Blacksmoke16 avatar Dec 20 '23 14:12 Blacksmoke16

Actually no, that branch fails on master of the DI component. It does pass on my refactor branch tho.

EDIT: I wouldn't worry about it. My refactor branch works fine and that'll be getting merged soon anyway.

Blacksmoke16 avatar Dec 20 '23 15:12 Blacksmoke16