-
Notifications
You must be signed in to change notification settings - Fork 10
Use log files in testing #375
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
base: master
Are you sure you want to change the base?
Conversation
|
This is a good idea to prevent our plugin from unnecessarily weighing. We also have to replace nexus files by its log files. |
da5df5b to
84b86a9
Compare
84b86a9 to
21c699f
Compare
21c699f to
9c7fb87
Compare
df6a792 to
132274b
Compare
132274b to
e3c9e28
Compare
|
I had an issue at the very beginning in FAIRmat with the log files, it came from float number precision related to hardware. The float numbers array came with different precision from different machines. I do not know how it works now. Do you think, we can also keep the test option with nexus_file, but we shall recommend using the log_file? |
I believe I made a change in the I am not a big fan of allowing the NeXus file as an alternative, because a) there should be one single way to do it, and b) there is a reason for not having the NeXus files - they are binary and thus there is not git difference, leading to unneccessary bloat of the plugin repos. |
64cd01e to
14a5c88
Compare
57e47c2 to
12f9897
Compare
Co-authored-by: RubelMozumder <[email protected]>
e72c257 to
a17b173
Compare
9413f01 to
62e2909
Compare
mkuehbach
left a comment
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.
Reasonable overall to move forward, but treatment of numeric edge cases is discussion-worthy, branch in need of sync with main, CI fixes
mkuehbach
left a comment
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.
Thanks for this addition, can be merged once updated by main and CI made passing.
This PR changes the
pynxtools.testingframework such that it compares theread_nexus logsdirectly during the tests. This avoids having to store binary NeXus files in the repos. Instead, we only commit reference logs, which will lead to much smaller diffs and keep the repositories lighter.For pynxtools-xps, I have a corresponding branch where this change in the testing is already implemented, including a script to autogenerate these log file consistently: https://github.com/FAIRmat-NFDI/pynxtools-xps/pull/69/files#diff-1ac04a8e7b33c27f6b8eea0d6f44313388de34a688758042f1ee9582f5b34ca2. This branch is also installed in the test_plugins pipeline here.