quil-lang / quilc

The optimizing Quil compiler.
Apache License 2.0
460 stars 72 forks source link

Handle quilc's global variables in the tests more nicely #192

Closed stylewarning closed 5 years ago

stylewarning commented 5 years ago

%entry-point mutates quilc::*protoquil* and quilc::*compute-matrix-reps*, which can cause future tests to be awry. We should audit this and make sure tests provide new bindings to mutate, or change the entry point to not mutate.

stylewarning commented 5 years ago

(@appleby ran into this issue during a PR they made.)

appleby commented 5 years ago

As far as I can tell, test-faithfulness is the only test that calls quilc::%entry-point.

Spitballing a couple of approaches:

1) Reload the file entry-point.lisp (or the whole :quilc system) on test completion. Most of the global state appears to be defparameter'ed, so in theory reloading should reset the state to default values. This is gross, but probably easy to implement.

2) Add some function like quilc::initialize-global-state or some such and call that both on program startup and on test exit.

3) Just setup the state required to run test-faithfulness in the body of the test, then call quilc::run-CLI-mode instead of quilc::%entry-point. That would skip calling quilc::process-options and avoid most of the state modifications. The upside of this approach is that it's robust against changes to process-options that add/mutate state unrelated to the test. The downside is that it's not robust against changes that add new global state relevant to the test. That is, if you change global state mutation in a way that affects computing the matrix reps, you might also need to change test-faithfulness.

4) Provide bindings in the test for any state that gets mutated by process-options. This is essentially what the test currently does, except that it binds only quilc::*protoquil* and quilc::*compute-matrix-reps* (sufficient to prevent downstream test failures, at time of writing), whereas process-options also mutates at least quilc::*human-readable-stream*, quilc::*quil-stream*, and quilc::*isa-descriptor*. This suffers from the same brittleness issues as (3).

5) Replace global state mutation with dynamic bindings in quilc::%entry-point (and anything it calls) wherever possible. This is a much bigger change since there is a fair bit of mutation going on. I have not looked closely enough to be sure if this would be possible and/or worth it. Possibly a yak not worth shaving.

stylewarning commented 5 years ago

Option 5 sounds like the "right" answer, but option 2 sounds like probably the most practical one.

appleby commented 5 years ago

I have a couple other small PRs I want to work on first, but if no else gets to it before then, I can take a whack at 5 and if it blows up in my face, fallback to 2.