-
Notifications
You must be signed in to change notification settings - Fork 173
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
refactor: Move common test helpers to core #2645
refactor: Move common test helpers to core #2645
Conversation
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 think this is it! Thanks @tboldagh
Tests/CommonHelpers/Acts/Tests/CommonHelpers/TestTrackState.hpp
Outdated
Show resolved
Hide resolved
@paulgessinger - would moving GenerateParameters.h to Core be still ok? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2645 +/- ##
==========================================
- Coverage 49.48% 48.68% -0.81%
==========================================
Files 474 478 +4
Lines 27075 27888 +813
Branches 12516 13172 +656
==========================================
+ Hits 13399 13578 +179
- Misses 4762 4765 +3
- Partials 8914 9545 +631 ☔ View full report in Codecov by Sentry. |
📊: Physics performance monitoring for 2a4d294physmon summary
|
I am not able to reproduce the compilation error enabling these: Any hint is welcome. |
Hmm, the test coverage is smaller. But how is that possible after just moving code around? |
@tboldagh The coverage calculation is not 100% robust. If it's a small change and we don't think it's damaging, we can ignore it. |
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.
Changes overall look clean. What do you think about putting the files into a subfolder detail_test
to indicate they're not really meant for public consumption?
Tests/CommonHelpers/Acts/Tests/CommonHelpers/MeasurementsCreator.hpp
Outdated
Show resolved
Hide resolved
@tboldagh looks good to me, only the format needs a quick fix, then we can merge this! |
This PR moves MultiTrajectory test helpers to the Core package. Thanks to that we would be able to reuse them in ATLAS to setup the compatible test. Once changes in MTJ implemenation in ACTS change it would be reflected in the test and during integration of that change the shared test will indicate what exacly needs to be updated.
@paulgessinger @CarloVarni - since you two discussed that a bit @ workshop please let me know if this is the right direction. As you see I have left out thet TestTrackProxy and TestSourceLink in order minimize polution of Core package.