search

CQCL / hugr-llvm

http://crates.io/crates/hugr-llvm
Apache License 2.0
5 stars 2 forks source link

test: Add integration tests lowering guppy programs #35

Closed doug-q closed 3 months ago

doug-q commented 3 months ago

Note that this PR includes #31 and #33, let's wait for those before we merge this.

doug-q commented 3 months ago

Ahh, I need to fix CI to have python with guppylang in path for the tests. EDIT: done

doug-q commented 3 months ago

The only thing I'm not sure about is if we should actually store the lowered Guppy examples as golden values. This only works if the node order outputted by Guppy is deterministic. AFAIK this is currently the case, but not sure if we want to guarantee that?

I think guppy node order should be deterministic, but it should also be unspecified. I.e. compiling the same program twice is guaranteed to give the same result, but it may change arbitrarily between guppy versions. I think it is worth some effort to ensure guppy is deterministic. There is certainly a lot of literature on the benefits of deterministic compilation and reproducible builds, and I would assume that the same arguments would apply to guppy.

I agree that these golden tests are fragile and that when they break it will not be guppy's fault. On the other hand, for now they are convenient, allow by-hand inspection of codegen in github, and I can't think of a better way to test that compiling guppy works. Are you ok with leaving them in for now, and replacing them when they become too annoying, or when we come up with a better solution? Maybe execution tests.

mark-koch commented 3 months ago

There is certainly a lot of literature on the benefits of deterministic compilation and reproducible builds, and I would assume that the same arguments would apply to guppy.

Fair enough. If we make that a Guppy requirement, we should probably verify it for all Guppy integration tests?

Are you ok with leaving them in for now, and replacing them when they become too annoying, or when we come up with a better solution? Maybe execution tests.

Yes, happy to merge the PR as is 👍