-
Notifications
You must be signed in to change notification settings - Fork 64
core: stdcm: fix path / travelled path confusion on block availability #11210
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
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #11210 +/- ##
=======================================
Coverage 81.42% 81.43%
=======================================
Files 1124 1124
Lines 113152 113153 +1
Branches 758 758
=======================================
+ Hits 92139 92141 +2
+ Misses 20958 20957 -1
Partials 55 55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
841c0e7 to
4ac834d
Compare
Erashin
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.
Well that must have been fun to debug. Good job, this is huge!
For the test, I'm fine with merging this as is, putting said test in an issue and doing it once the other PRs are merged.
core/src/test/kotlin/fr/sncf/osrd/stdcm/preprocessing/BlockAvailabilityTests.kt
Outdated
Show resolved
Hide resolved
4ac834d to
cb1ebc5
Compare
bougue-pe
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 bugfix!
Apart from the naming, I guess my first 2 comments can (should?) be tracked a sibling issues of #10595.
If agreeing on the usefulness of suggestions, I can create issues.
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt
Show resolved
Hide resolved
core/src/main/kotlin/fr/sncf/osrd/stdcm/infra_exploration/InfraExplorerWithEnvelopeImpl.kt
Show resolved
Hide resolved
core/src/test/kotlin/fr/sncf/osrd/stdcm/preprocessing/BlockAvailabilityTests.kt
Outdated
Show resolved
Hide resolved
core/src/test/kotlin/fr/sncf/osrd/stdcm/preprocessing/BlockAvailabilityTests.kt
Outdated
Show resolved
Hide resolved
6698feb to
977f646
Compare
18c0fb6 to
ca7af9e
Compare
Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>
ca7af9e to
9ba6aab
Compare
This early return relied on the fact that the nodes with lowest total time were processed first, which isn't true anymore Signed-off-by: Eloi Charpentier <eloi.charpentier.42@gmail.com>
bougue-pe
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.
🙏 If you have a unit test for the second commit's bug, it's nice to embed (we can consider that e2e tests are enough, I guess).
Well that was a massive bug considering how sneaky it is. It's in
BlockAvailability(a class that's mocked in most tests), andOffset<Path> == Offset<TravelledPath>in most test cases.InfraExplorerWithEnvelope.getSimulatedLength()returned aLength<Path>, when it should really beTravelledPath.I'd add a unit test with stops in
BlockAvailabilityTests, but that would massively conflict with the other pending PRs.Fix some (most?) cases of #11121.