Skip to content

Conversation

@JessieFrance
Copy link
Contributor

This PR is for #1022, and aims to expand testing for mkdir -p when (1) a sub-directory already exists, and (2) the sub-directory does not exist. Please let me know if you'd like me to change something, and thank you for the opportunity to work on this issue!

@nfischer
Copy link
Member

Hmm for some reason it looks like the coverage report isn't changing for mkdir.js: https://app.codecov.io/gh/shelljs/shelljs/compare/1026. Do you know why this condition doesn't appear to be covered?

shelljs/src/mkdir.js

Lines 30 to 33 in 71085db

} catch (e) {
// swallow error if dir already exists
if (e.code !== 'EEXIST' || common.statNoFollowLinks(dir).isFile()) { throw e; }
}


It looks like appveyor is not reporting coverage info, as Windows-specific code doesn't appear to have coverage. This must be a codecov or appveyor bug though.

@JessieFrance
Copy link
Contributor Author

Thank you for looking at this PR and for creating this project!

I'm not sure on the appveyor/Windows part, but after looking at the code block above, and investigating/logging with npm run test-with-coverage, I learned that no tests currently trigger those lines. Although I would've thought the mkdir -p test on an existing subdirectory might have, it appears this _mkdir function returns that test case early here before we can get to the mkdirSyncRecursive function call:

return; // skip dir

To check this, if I re-write that line on my local branch as if (!options.fullpath || (options.fullpath && stat.isFile())) return; then the test coverage goes up (although not to perfect 100s everywhere for mkdir). There is also possibly some duplicated logic where even with that fix we still never reach inside this if statement:

if (e.code !== 'EEXIST' || common.statNoFollowLinks(dir).isFile()) { throw e; }

Should I add to this PR (by changing shelljs/src/mkdir.js), or possibly file another issue or PR to increase the test coverage?

@nfischer
Copy link
Member

nfischer commented Apr 1, 2021

I see, thanks for looking into this. Let's land this as-is because it adds useful test coverage. If you can think of a way to de-duplicate the logic, that would be a welcome contribution in a separate PR!

Thank you!

@nfischer nfischer merged commit 124d334 into shelljs:master Apr 1, 2021
@JessieFrance JessieFrance deleted the mkdir-p-tests branch June 24, 2021 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants