Skip to content

Commit

Permalink
fix: refactoring to pass tests on Windows
Browse files Browse the repository at this point in the history
This is a larger refactoring than I tend to prefer to do in a single
commit, but here goes.

- The path normalization of \ to / is made more comprehensive.

- Checking to ensure we aren't overwriting the cwd is done earlier in
  the unpack process, and more thoroughly, so there is less need for
  repetitive checks later.

- The cwd is checked at the start in our recursive mkdir, saving an
  extra fs.mkdir call which would almost always result in an EEXIST.

- Many edge cases resulting in dangling file descriptors were found and
  addressed.  (Much as I complain about Windows stubbornly refusing to
  delete files currently open, it did come in handy here.)

- The Unpack[MAKEFS] methods are refactored for readability, and no
  longer rely on fall-through behavior which made the sync and async
  versions slightly different in some edge cases.

- Many of the tests were refactored to use async rimraf (the better to
  avoid Windows problems) and more modern tap affordances.

Note: coverage on Windows is not 100%, due to skipping many tests that
use symbolic links.  Given the value of having those code paths covered,
I believe that adding istanbul hints to skip coverage of those portions
of the code would be a bad idea.  And given the complexity and hazards
involved in mocking that much of the filesystem implementation, it's
probably best to just let Windows not have 100% coverage.
  • Loading branch information
isaacs committed Aug 9, 2021
1 parent ce5148e commit 166cfc0
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 3 deletions.
1 change: 1 addition & 0 deletions .gitattributes
@@ -0,0 +1 @@
test/fixtures/files/** text eol=lf
2 changes: 1 addition & 1 deletion lib/write-entry.js
Expand Up @@ -80,8 +80,8 @@ const WriteEntry = warner(class WriteEntry extends MiniPass {
if (!this.preservePaths) {
const s = stripAbsolutePath(this.path)
if (s[0]) {
this.warn('stripping ' + s[0] + ' from absolute path', this.path)
this.path = s[1]
this.warn('stripping ' + s[0] + ' from absolute path', p)
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/pack.js
Expand Up @@ -145,7 +145,7 @@ t.test('pack a dir', t => {
cksumValid: true,
needPax: false,
path: 'dir/',
mode: 0o755,
mode: Number,
size: 0,
mtime: null,
cksum: Number,
Expand Down Expand Up @@ -173,7 +173,7 @@ t.test('pack a dir', t => {
cksumValid: true,
needPax: false,
path: 'dir/x',
mode: 0o644,
mode: Number,
size: 0,
mtime: mtime,
cksum: Number,
Expand Down

0 comments on commit 166cfc0

Please sign in to comment.