-
Notifications
You must be signed in to change notification settings - Fork 19
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
Correctness testing with TOML #1682
Conversation
This fixes the last three round trip issues with TOML. I've marked those tests as working.
This should probably be cleaned up later
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.
All looks good. Those failing tests are somewhat annoying, though!
Couple of comments, nothing major.
tests/testData.h
Outdated
CoreData other_; | ||
Dissolve trial_{other_}; |
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.
These names are perhaps a bit too generic! I might suggest:
CoreData other_; | |
Dissolve trial_{other_}; | |
CoreData otherCoreData_; | |
Dissolve otherDissolve_{otherCoreData_}; |
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've gone ahead and renamed them otherCoreData
and otherDissolve
. I dropped the trailing underscore because these aren't members of the class, but just variables in the function.
std::vector<std::shared_ptr<AtomType>> &types) | ||
: range_(range), delta_(delta), atomTypeChargeSource_(source), atomTypes_(types), | ||
coulombTruncationScheme_(PairPotential::coulombTruncationScheme_), | ||
SerializablePairPotential::SerializablePairPotential(double &range, double &delta, bool &source, bool &forceCharge, |
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.
Do we still need the SerializablePairPotential
class? It feels like its just wrapping functionality that could be added into Dissolve::serialise()
and Dissolve::deserialise()
...
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 agree that we need to review SerializablePairPotential
, as I'm increasingly unhappy with the design. However, I'm not sure that Dissolve
is the right place for it, either. I'll add an issue for it and we can review that before the final TOML addition.
Now that TOML properly performs the round trip, we need to test for correctness. This PR performs the following changes
a. We still perform the round trip testing to prevent regressions
b. Tests tagged with TomlFailure only run the original input file unless the toml testing flag is activated. This marks the 17 tests currently failing under TOML