Regression test for sandboxed evaluation #173
Conversation
This was recently fixed by handling the runtime error resulting from requiring the macro debugger library (which is due to the sandboxed evaluator disallowing macro introspection). This adds a regression test that validates the bug and the fix.
|
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. |
|
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? |
|
Made one last change -- now that it's just using |
I mean that dd17c10 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
(I think I've gotten all the related work together?) |
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 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. |
|
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. |
|
Closing in favor of #176 |
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-debuggerlibrary inside a sandboxed evaluator.Although the test validates the bug as well as the fix, the coverage checker was still showing the
lambdaused 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
(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.)