-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
reftests: add test for operators in opamfile filters #5642
Conversation
4fc119c
to
8462183
Compare
8462183
to
6d32dc6
Compare
dfe6255
to
1798862
Compare
1798862
to
364f9c6
Compare
364f9c6
to
f50596c
Compare
- install c 1 [required by gteq] | ||
- install gteq 1 | ||
- install ok 1 [required by gteq] | ||
### :: Combination :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's definitely good to have some tests for this, but I'm not sure that reftests are the correct harness? Wouldn't it be better to be testing the filter parser then evaluation of filters separately (alcotest-style or some such), rather than invoking the whole of opam machinery to test this?
I'm pretty sure the tests above this comment would definitely be clearer as a standard test case harness and I'm fairly even the tests below would be?
(i.e. I think the reftests framework is excellent for full-blown tests, but these feel more like unit tests and the framework's overkill for that?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to have in the reftest since it's something that is usable and most used from the command line tool
f50596c
to
9722749
Compare
Thanks! |
No description provided.