Skip to content

Commit

Permalink
test sandboxed eval with macro introspection
Browse files Browse the repository at this point in the history
This adds a regression test for the recent issue [1] 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. 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 regression test validates the bug and 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 [2] 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.

[1]: https://github.com/drym-org/qi/wiki/Qi-Meeting-Mar-29-2024
[2]: https://docs.racket-lang.org/cover/api.html#%28def._%28%28lib._cover%2Fmain..rkt%29._irrelevant-submodules%29%29
  • Loading branch information
countvajhula authored and benknoble committed Jun 16, 2024
1 parent 515122f commit 8bff3b5
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 3 deletions.
11 changes: 9 additions & 2 deletions qi-lib/flow/core/passes.rkt
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
(provide (for-syntax define-and-register-pass
run-passes))

(begin-for-syntax
(module macro-debug racket/base
;; See tests/compiler/rules/full-cycle.rkt for an explanation
;; re: sandboxed evaluation, submodules and test coverage.
(provide my-emit-local-step)

(define my-emit-local-step
;; See "Breaking Out of the Sandbox"
Expand All @@ -15,7 +18,11 @@
(make-keyword-procedure
(lambda (kws kw-args . rest)
(void))))))
(dynamic-require 'macro-debugger/emit 'emit-local-step)))
(dynamic-require 'macro-debugger/emit 'emit-local-step))))

(require (for-syntax 'macro-debug))

(begin-for-syntax

;; Could be a list but for future extensibility a custom struct is
;; probably a better idea.
Expand Down
1 change: 1 addition & 0 deletions qi-test/info.rkt
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@
(define build-deps '("rackunit-lib"
"adjutor"
"math-lib"
"sandbox-lib"
"qi-lib"))
(define clean '("compiled" "tests/compiled" "tests/private/compiled"))
25 changes: 24 additions & 1 deletion qi-test/tests/flow.rkt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
racket/string
racket/function
racket/format
racket/sandbox
(except-in "private/util.rkt"
add-two)
syntax/macro-testing)
Expand Down Expand Up @@ -1657,7 +1658,29 @@
(thunk
((☯ (== _ _ _))
3)))
(test-equal? "relay-_" ((☯ _) 3) 3))))))
(test-equal? "relay-_" ((☯ _) 3) 3))))

(test-suite
"regression tests"
(test-suite
"sandboxed evaluation"
(test-not-exn "Plays well with sandboxed evaluation"
;; See "Breaking Out of the Sandbox"
;; https://github.com/drym-org/qi/wiki/Qi-Meeting-Mar-29-2024
;;
;; This test reproduces the bug and the fix fixes it. Yet,
;; coverage does not show the lambda in `my-emit-local-step`
;; as being covered. This could be because the constructed
;; sandbox evaluator "covering" the code doesn't count as
;; coverage by the main evaluator running the test?
;; We address this by putting `my-emit-local-step` in a
;; submodule, which, by default, are ignored by coverage.
(lambda ()
(let ([eval (make-evaluator
'racket/base
'(require qi))])
(eval
'(☯ add1)))))))))

(module+ main
(void (run-tests tests)))

0 comments on commit 8bff3b5

Please sign in to comment.