ADD: First set of nuclearizer unit tests#150
Conversation
|
EDIT: Never mind, this was resolved by updating megalib.. Should've read your initial message more carefully..
|
|
Did you update MEAGlib? |
|
I did now, and it works... would have helped if I had fully read your initial message 😅 Sorry for the fuzz.. |
fhagemann
left a comment
fhagemann
left a comment
There was a problem hiding this comment.
A had a quick look over all files, and especially for the .cxx files in unittests, I only browsed through them instead of reading them line by line.
Regarding your comments:
(1) The bug fixes look fine --> see my comments in those files if there are any
(2) Test data also looks fine, except for some .cfg files that have paths that link to some files other than this test data. I believe that shouldn't be an issue though
(3) These unit level tests are VERY detailed ^^ good thing we have AI helping us out here. I think I had one comment on adding tests for reading TI from ROA files and making sure that we get the time assignment to m_EventTimeUTC correct. Also I was wondering why we use sometimes 0u and sometimes (unsigned int) 0
(4) I actually read through the TestingConventions.md in a bit more detail and I think I also learned quite a number of new things from the C++ testing cosmos and good practice for writing unit tests. I recommend this for others to also have a look if they have time ;)
| // IsXStrip is an alias for IsLowVoltageStrip | ||
| H.IsXStrip(true); | ||
| Passed = EvaluateTrue("IsXStrip(bool)/IsXStrip()", "alias true", "IsXStrip() returns true after IsXStrip(true)", H.IsXStrip()) && Passed; | ||
| Passed = EvaluateTrue("IsXStrip(bool)/IsLowVoltageStrip()", "alias true", "IsLowVoltageStrip() returns true after IsXStrip(true)", H.IsLowVoltageStrip()) && Passed; | ||
| H.IsXStrip(false); | ||
| Passed = EvaluateFalse("IsXStrip(bool)/IsXStrip()", "alias false", "IsXStrip() returns false after IsXStrip(false)", H.IsXStrip()) && Passed; |
There was a problem hiding this comment.
Will IsXStrip be discontinued at some point?
There was a problem hiding this comment.
yes... we have to do more cleanup in this class
|
I also ran some of the tests. Do you expect some of them to fail? |
|
There was an issue with the tests on Linux which did not show up on macOS. Fixed. They all pass now. |
|
I think I fixed all your suggestion except the style fixes, which are the next pass over the classes. |
|
For me, the 
|
|
I also resolved all of my comments (except for the clean-up ones for reference) |
| MReadOutAssembly R; | ||
| R.SetID(10); | ||
| MPhysicalEvent* PE = new MPhysicalEvent(); | ||
| R.SetPhysicalEvent(PE); | ||
| delete PE; // SetPhysicalEvent duplicated it, so the caller still owns PE |
There was a problem hiding this comment.
This MPhysicalEvent has m_EventType == -1 (Unknown), causing my unit tests to fail.
There was a problem hiding this comment.
Fixed it in MEGAlib
| Passed = EvaluateTrue("StreamRoa()", "UH line present with strip hit", "StreamRoa() output contains 'UH' when there is a strip hit", | ||
| S.Contains("UH")) && Passed; |
There was a problem hiding this comment.
Related to #156: should only MStripHits start their line with UH?
| R.SetID(6); | ||
| MCrystalHit* CH = new MCrystalHit(); | ||
| CH->SetCrystalID(2); | ||
| CH->AddOrigins({77}); | ||
| R.AddCrystalHit(CH); | ||
|
|
||
| // WithOrigins=false: the crystal hit origin 77 must not appear | ||
| ostringstream SS_no; | ||
| R.StreamRoa(SS_no, true, true, false, false, false, false, false, false); | ||
| MString S_no = SS_no.str(); | ||
| Passed = EvaluateTrue("StreamRoa()", "crystal WithOrigins=false omits origins", "StreamRoa() with WithOrigins=false does not emit the crystal hit origin 77", | ||
| S_no.Contains("77") == false) && Passed; |
There was a problem hiding this comment.
Can we also check here for MCrystalHit if the line starts with "UH" (or "UC" in case that #156 is the way it was intended)?
|
I fixed the crash issue and on other issue in MEGAlib. |
|
With the newest version of MEGAlib, I don't get that error anymore, and all tests pass now: |
|
Thanks, Felix! |
2 participants
This PR adds the first structured nuclearizer unit-test suite and the required test data (in resource/unittestdata). It covers core readout classes (MStripHit, MHit, MReadOutAssembly) plus one module for testing, MModuleLoaderMeasurementsHDF. It also adds end-to-end tests for the 542-1 and 406-1 datasets.
First, you need to update MEGAlib to the latest version, since I offloaded some added functionality into the best suited MEGAlib base classes.
Then, you compile nuclearzier and the unit tests via
This creates a bunch of files which start via "UTN*", e.g. UTNStripHit. Those are the unit tests.
You can run them individually, e.g.,
or all together via:
Try it out!
There are 4 focus areas for the review:
(1) I did a bug fix passes on MReadOutAssembly, MStripHit, and MHit. I did not change the style, and ignored that our StreamDat functionality is a mess. I will fix that next after this initial review. There are lots of changes, in those classes - but their where mostly minor issues, and the output of the unit tests is unchanged
(2)
The test data is in resource/unittests. Please feel free to take a look what we have so far - we need lots more in the future.
(3)
The unit tests live in the directory unittests. All the tests are AI generated. I did some course review, and had the AI correct some stuff. You can take a quick look for completeness (and nonsense - which gets generated too - but I hope I found all).
(4)
The unit-test directory has a file called TestingConventions.md. The first part tells you how to prompt an AI agent to make more unit tests, and the second part are instructions from an AI agent for an AI agent on how to make the tests.