-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
First attempt at refactoring Oracle
s
#2709
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2709 +/- ##
==========================================
+ Coverage 78.51% 78.53% +0.01%
==========================================
Files 461 463 +2
Lines 15885 15904 +19
Branches 2609 2590 -19
==========================================
+ Hits 12472 12490 +18
- Misses 3413 3414 +1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
This is a nice PR. It looks nice, structured, and extremely well commented. How about adding a bunch of unit tests?
* An oracle that uses an integer variable. Although using integers as an oracle is the most straightforward decision, | ||
* do not use this oracle by default. It is handy, when reasoning about sequences. | ||
* | ||
* TODO: add tests |
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.
How about adding tests? :)
This should be super-easy with the new design. Just a bunch of unit tests
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.
Well, I sort of ran out of time before PI week, but yes, adding tests is on my todo list
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.
Looks great, I really appreciate the clarity in code + docs!
Could we add the missing unit tests here? It would make for a more self-contained PR.
...cmt/src/main/scala/at/forsyte/apalache/tla/bmcmt/stratifiedRules/aux/oracles/IntOracle.scala
Outdated
Show resolved
Hide resolved
...cmt/src/main/scala/at/forsyte/apalache/tla/bmcmt/stratifiedRules/aux/oracles/IntOracle.scala
Outdated
Show resolved
Hide resolved
tla-bmcmt/src/main/scala/at/forsyte/apalache/tla/bmcmt/stratifiedRules/aux/oracles/Oracle.scala
Outdated
Show resolved
Hide resolved
…iedRules/aux/oracles/Oracle.scala Co-authored-by: Thomas Pani <[email protected]>
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.
LGTM
make fmt-fix
(or had formatting run automatically on all files edited)./unreleased/
for any new functionalityFirst step towards #2708.
The PR makes the following changes to https://github.com/informalsystems/apalache/blob/6bb36acc1ad88fdeba21d45b67663b1e792e29c6/tla-bmcmt/src/main/scala/at/forsyte/apalache/tla/bmcmt/rules/aux/Oracle.scala:
SymbState
is replaced byRewriterScope
caseAssertions
has been changed such that the optional sequence is now anOption
, instead of treating the empty sequence as a special case.getIndexOfOracleValueFromModel
(previouslyevalPosition
) has been changed, to no longer includestate
. Upon inspection, none of the oracle implementations ever accessed this parameter.IntOracle
is implemented with the changes above as a prototype. Note that tests are currently missing for the IntOracle, and will be added in a subsequent PR.