-
Notifications
You must be signed in to change notification settings - Fork 15
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
155 fix dependency loop between models and minimizer #183
155 fix dependency loop between models and minimizer #183
Conversation
Strictly speaking this is a breaking change as we remove a public function. We can however accept this possibility and make this new library once someone raises an issue |
8d5b61f
to
6bf4a3e
Compare
Okay, I redid the implementation. For some reason, the checks are not appearing on the PR page but are being done anyway... |
It seems the automatic cmake formatter does not trigger the workflows. I will have a look at it |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #183 +/- ##
===========================================
+ Coverage 82.81% 83.42% +0.60%
===========================================
Files 54 54
Lines 19384 19385 +1
Branches 1852 1852
===========================================
+ Hits 16053 16172 +119
+ Misses 3331 3213 -118
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 good so far. But maybe we could add a unit test for the missing code coverage?
As this is now a free function we could even use gmock for that
Free functions are not "mockable" out of the box. Should I use an interface or convert everything into an actual class? |
No, I meant you could implement a mock of the Or you can use an existing class like the CH2DM or so |
Okay, I used the C2HDM for the I am not understanding how to use |
It's basically two different points: If we want to ensure that the c2hdm model passes the check implementation function, then we use the c2hdm directly. If we want to ensure that the CheckImplementation fails when it should fail and pass when it should pass, gmock is an easy option as we can change just one ON_CALL for the test I think for now you can drop the other PR, but sometime in the future it would be nice to ensure that these functions, which should be used by users implementing their own model, work in general and not only in our models. |
ModelID::FChoose(ModelID::ModelIDs::C2HDM, SMConstants); | ||
modelPointer->initModel(example_point_C2HDM); | ||
|
||
ModelTests::CheckImplementation(*modelPointer, |
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 does not check if the checks passed or failed. I added a given check
Moved
CheckImplementation
intosrc/prog/Test.cpp
. Now we the libraryModels
does not depend onMinimizer
.It seemed like an overkill to create a library just for this.