Skip to content
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

MockOracle refactor #2731

Merged
merged 12 commits into from
Jan 3, 2024
Merged

MockOracle refactor #2731

merged 12 commits into from
Jan 3, 2024

Conversation

Kukovec
Copy link
Collaborator

@Kukovec Kukovec commented Sep 13, 2023

  • Tests added for any new code
  • Ran make fmt-fix (or had formatting run automatically on all files edited)
  • Documentation added for any new functionality
  • Entries added to ./unreleased/ for any new functionality

Following the pattern laid out in #2709 for IntOracle, this PR introduces the same alterations to MockOracle.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (253f4c4) 78.86% compared to head (e37496b) 78.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2731      +/-   ##
==========================================
- Coverage   78.86%   78.84%   -0.02%     
==========================================
  Files         466      467       +1     
  Lines       15923    15930       +7     
  Branches     2557     2553       -4     
==========================================
+ Hits        12557    12560       +3     
- Misses       3366     3370       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kukovec Kukovec mentioned this pull request Oct 17, 2023
4 tasks
@Kukovec Kukovec requested a review from thpani October 20, 2023 12:27
Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We should track re-enabling these commented-out tests though, or we might forget.

Comment on lines 129 to 130
// Ignored until we figure out why it's killing GH CLI
ignore("getIndexOfChosenValueFromModel recovers the index correctly") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we

  • refer to the relevant issue here, and
  • leave a note under that issue to (re-)enable these tests?

check(prop, minSuccessful(1000), sizeRange(4))
}

// We don't actually need the solver in MockOracle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this you mean, we are passing it, but it's not actually invoked?

Comment on lines 73 to 74
// Ignored until we figure out why it's killing GH CLI
ignore("getIndexOfChosenValueFromModel recovers the index correctly") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we

  • refer to the relevant issue here, and
  • leave a note under that issue to (re-)enable these tests?

@shonfeder
Copy link
Contributor

Looks like the CI failures were just transient issues in the GH runners. I'll fix the comments and get this merged.

@shonfeder shonfeder merged commit 70d7f24 into main Jan 3, 2024
10 checks passed
@shonfeder shonfeder deleted the jk/oracles branch January 3, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants