-
Notifications
You must be signed in to change notification settings - Fork 60
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
Fixed unit tests on Windows machines #55
Conversation
* Added getWorkingRoot function which returns the root directory of CWD; replaced references to the root system mount point "/" with references to getWorkingRoot. * Replaced hard-coded references to Linux filesystem features with dynamic references dependent on process.platform or existing platform-agnostic functions. * Added missing configuration options for mock-fs to avoid dirent re-creation errors.
Hey, thank you so much for working on this! I don't have a Windows machine; can you help me setup CI on Windows in this PR or another PR so we can confirm this works? |
const api = new fdir().crawl("/etc"); | ||
const secureDir = | ||
process.platform === 'win32' | ||
? 'C:\\Windows' |
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.
Are we sure it will always be C:\\Windows
?
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 would say it's safe to assume Windows is installed on the C:
drive. While it is technically possible to run windows off of another drive, this is extremely rare. This could be addressed in the future once a contributor actually needs the unit tests to support crawling the Windows
directory from another drive.
Once this PR gets merged, I'm happy to help get the CI working for Windows. I got it working using a fork of this branch here (notice that they're all passing). Once the conflicts are resolved in this PR and it gets merged, I will submit a proper PR for the CI build script. |
@eulerfields there are some conflicts with the master branch. Would you be able to resolve them so that this PR can be merged? If not, I'm happy to pick up the branch and submit a new PR with these fixes and the fixes for the merge conflicts. |
All tests are now passing on all platforms after #81 |
replaced references to the root system mount point "/" with references
to getWorkingRoot.
references dependent on process.platform or existing platform-agnostic
functions.
re-creation errors.