Regression test for sandboxed evaluation

[countvajhula]

Summary of Changes

This adds a regression test for the recent issue where building docs for packages depending on Qi was failing due to the use of macro introspection functionality in the macro-debugger library inside a sandboxed evaluator.

Although the test validates the bug as well as the fix, the coverage checker was still showing the lambda used in "monkey patching" the macro debugger utility as uncovered by tests. My guess is that this is because the coverage tool only counts code as covered if the evaluator used in the test runner evaluates those lines, whereas in the present case, the code is "covered" by a sandboxed evaluator we construct in the test rather than the test runner itself.

Assuming this is the right explanation, I've placed the original fix inside a submodule -- by default, the coverage checker ignores all submodules for the purposes of coverage. So in cases where we are sure some code needn't be covered (or which we know is covered but isn't detected, as in this case), we can use this recipe as a standard way to add an exclusion to coverage.

Public Domain Dedication

• [x] In contributing, I relinquish any copyright claims on my contribution and freely release it into the public domain in the simple hope that it will provide value.

(Why: The freely released, copyright-free work in this repository represents an investment in a better way of doing things called attribution-based economics. Attribution-based economics is based on the simple idea that we gain more by giving more, not by holding on to things that, truly, we could only create because we, in our turn, received from others. As it turns out, an economic system based on attribution -- where those who give more are more empowered -- is significantly more efficient than capitalism while also being stable and fair (unlike capitalism, on both counts), giving it transformative power to elevate the human condition and address the problems that face us today along with a host of others that have been intractable since the beginning. You can help make this a reality by releasing your work in the same way -- freely into the public domain in the simple hope of providing value. Learn more about attribution-based economics at drym.org, tell your friends, do your part.)

[benknoble]

P.S. I'm glad you were able to write a test that would fail and now passes; I would very much prefer if the useful detail in your PR description were contained in the commit that added the test.

[countvajhula]

These are all great and keen observations as always. I've made all the changes and validated that the test still recognizes regression.

re: commit message, this is the one that added the test (not the initial commit) and it does include the info about it validating the regression. Is that what you mean?

[countvajhula]

Made one last change -- now that it's just using require qi, it seemed like a dedicated regression tests suite at the level of the library would make sense. Moved the test there and modified it to just use flow rather than ~>. FTR, attempting to use (flow (~> ...)) inside a sandboxed evaluator produces a different error, that the sandbox is not allowed to access the unexported binding ~>. I seem to recall we ran into that before and it's a known bug with binding spaces and sandboxed evaluation. Ah yes, it's this one. So I've modified the test to just use a simple function identifier, which still reproduces the bug and validates the fix.

[benknoble]

re: commit message, this is the one that added the test (not the initial commit) and it does include the info about it validating the regression. Is that what you mean?

I mean that dd17c10b4ac83800bad6cde5f6ffbf96e5a7236c should include some (most? all?) of the information from your PR description (the links, context, and explanation of technical choices are all helpful and unlikely to be captured meaningfully within Git).

I'm trying to encourage more folks to pay as much attention to their commits as they do to their code :)

To that end, before merging this I would expect

• 8921f31 and 7f0032d to be squashed into dd17c10b4ac83800bad6cde5f6ffbf96e5a7236c, which also gets an improved message
• a1b542a, b09ddd0, and f3e3a65 to be squashed into 52f5412

(I think I've gotten all the related work together?)

[benknoble]

To that end, before merging this I would expect

Here's what that might look like (feel free to use it directly for this PR, if you like): https://github.com/benknoble/qi/tree/test-sandboxed-eval

I did git rebase --interactive main and changed the todo list to be

pick 0ba5f9a lint: Don't re-import the same bindings from two modules
pick 52f5412 More direct "full cycle" compiler test macro
fixup a1b542a cr: remove unused `test-passes~>` macro
fixup b09ddd0 cr: make `qi-compile` a function
fixup f3e3a65 cr: remove unused requires
reword dd17c10 Add a regression test for sandboxed eval with macro introspection
fixup 8921f31 cr: simplify sandboxed eval test
fixup 7f0032d Move regression test up to `flow` tests into a dedicated suite

The only manual step in the list was to reword the one commit; I pasted your PR description into the existing description and tidied up.

[countvajhula]

You're right! We should be paying more attention to commit messages as they allow our processes to support us better in the long run. I've been relying on PR descriptions to fill out the paper trail, but perhaps that isn't wise as it's tied to a specific provider rather than the source itself. I'm happy to merge your curated version, and thank you.

[countvajhula]

Closing in favor of #176