Closed kt218 closed 2 years ago
@kt218 it's nice to have more tests. However, I am concerned that some of the checks depend on the LLVM version and might fail when we upgrade to a newer version. In particular, the number of LLVM instructions (33) might change. I think it would be better to remove those checks and only leave those which are more stable.
@ccadar I can change the e2e test so that it would take the stdout number of instructions and compare it with the value from stats. Or should this just be part of the runner test?
@kt218 I think that would be a more flexible check, yes. I think some LLVM version-dependent tests are fine too, but they should be clearly documented and made obvious/easy to update when we upgrade to a newer LLVM. In this case, I think your proposal is better.
@ccadar I've modified the e2e stat test to compare with instr count from stdout. Should I also change the original tests to use regex instead of absolute matches to a string?
LGTM!
Hi @kt218 great, let's merge this now. If you want, you can open a different PR to improve the old tests, and we can discuss them there.
Added e2e tests that tests:
Added runner tests that tests: