Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run spec suite in macro finished hook instead of at_exit #14121

Open
straight-shoota opened this issue Dec 20, 2023 · 4 comments · May be fixed by #14122
Open

Run spec suite in macro finished hook instead of at_exit #14121

straight-shoota opened this issue Dec 20, 2023 · 4 comments · May be fixed by #14122

Comments

@straight-shoota
Copy link
Member

straight-shoota commented Dec 20, 2023

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 (#13763 (comment)).

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
@Blacksmoke16
Copy link
Member

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?

@straight-shoota straight-shoota linked a pull request Dec 20, 2023 that will close this issue
@straight-shoota
Copy link
Member Author

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 🚀

@Blacksmoke16
Copy link
Member

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
Copy link
Member

Blacksmoke16 commented Dec 20, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants