-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Fix SimpleModuleLoader on Windows #3795
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3795 +/- ##
==========================================
+ Coverage 47.24% 50.22% +2.98%
==========================================
Files 476 456 -20
Lines 46892 44823 -2069
==========================================
+ Hits 22154 22514 +360
+ Misses 24738 22309 -2429 ☔ View full report in Codecov by Sentry. |
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.
To avoid the same error in the future, maybe the test should be changed to test the path components instead of the whole path string.
EDIT: Just realized that this change could also deduplicate the tests.
I don't think we would gain anything from checking the path components, but I'll make the change. There was a bug in the code and the tests were built to test the bug, so both were faulty. It happens. (as proof that the bug was in the test AND the code, I didn't change the expected value, just the argument itself) |
But the bug passed the test because we were using separators on the test itself, right? If I'm correct, testing by components instead of using separators would have caught the bug. |
The bug was two fold:
I might be over explaining. We can sync offline tomorrow morning if you or I are missing something. Cheers! |
That makes a lot of sense. Thank you for explaining! |
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.
Looks good to me! :)
Basically I made a mistake with my previous PR and discovered it as I was building
embed_module!
.