-
Notifications
You must be signed in to change notification settings - Fork 25
add test for testing UserContextWinDisk to ensure it won't get broken #158
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
|
@BrianPugh this PR shouldn't be merged, seems like the new test was skipped also on windows. |
|
@BrianPugh so there were bugs in UserContextWinDisk that prevented it to work. |
I'm a bit conflicted; if it was indeed broken and noone has reported it, then it certainly doesn't seem well used. However, I also know that a lot of embedded people end up doing development on Windows machine (for one reason or another). Also, @jrast might be a windows-user, which might be why this class exists in the first place, so I wouldn't want to remove functionality that they specifically developed. It's also entirely possible that people who depend on this feature are pinned to a quite-old version of littlefs-python in a "if it ain't broken, don't update it" mindset. Maybe it works in certain environments? I'm leaning towards merging in your fix (and make sure that the test always runs if the os is |
|
The UserContextWinDisk was added by @Jiajun-Huang back in 2023 (a286289). So mabe some input from his / her side? Honestly, I'm not using littlefs-python myself since a longer time (based on the projects we work on). However, a huge thank you to all of you, mostly @BrianPugh for keeping the package up to date! |
|
Hi @BrianPugh , I think I understand the confusing part here (if the code is really broken, why it was inserted): Hence removing it totally is really not the correct solution, options are to merge my changes (and really support win32file writes, is it really needed or just adding complexity and corner cases?) or to prevent write operations into filesystem that uses the windows context (I think I can provide such PR). |
BrianPugh
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.
The fix looks simple enough, let's go ahead with this PR with just 1 slight change. Thanks for doing this!
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not HAS_WIN32FILE or sys.platform != "win32", |
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.
let's remove the HAS_WIN32FILE check and just have this only be conditional on if the sys.platform is win32. I don't think we need to import win32file at all within this test and it should all "just work" on all platforms.
I think as we have this context option it is important to add test to ensure it is never be broken (totally at least).
To be honest, I am not on windows so I didn't checked it is working (my AI claim there are multiple problems in UserContextWinDisk, let see if he right)