Skip to content

Implement GH actions - #881

Merged
RyanZim merged 4 commits into
masterfrom
ryan/gh-actions
May 1, 2021
Merged

RyanZim merged 4 commits into
masterfrom
ryan/gh-actions

Conversation

@RyanZim

@RyanZim RyanZim commented Apr 1, 2021

Copy link
Copy Markdown
Collaborator

No description provided.

@JPeer264 JPeer264 mentioned this pull request Apr 9, 2021
@RyanZim

RyanZim commented Apr 21, 2021 •

Copy link
Copy Markdown
Collaborator Author

For some reason, fs.symlink*() itself seems to behave differently in Windows on GH Actions than in AppVeyor, breaking the tests.

@RyanZim

RyanZim commented Apr 22, 2021

Copy link
Copy Markdown
Collaborator Author

The tests that are breaking aren't actually testing fs-extra, they're testing Node itself. This diff would fix tests:

diff --git a/lib/ensure/__tests__/symlink.test.js b/lib/ensure/__tests__/symlink.test.js
index 22a0043..477b084 100644
--- a/lib/ensure/__tests__/symlink.test.js
+++ b/lib/ensure/__tests__/symlink.test.js
@@ -352,24 +352,6 @@ describe('fse-ensure-symlink', () => {
     })
   }
 
-  describe('fs.symlink()', () => {
-    const fn = fs.symlink
-    tests.forEach(test => {
-      const args = test[0].slice(0)
-      const nativeBehavior = test[1]
-      // const newBehavior = test[2]
-      if (nativeBehavior === 'file-success') fileSuccess(args, fn)
-      if (nativeBehavior === 'file-broken') fileBroken(args, fn)
-      if (nativeBehavior === 'file-error') fileError(args, fn)
-      if (nativeBehavior === 'file-dest-exists') fileDestExists(args, fn)
-      args.push('dir')
-      if (nativeBehavior === 'dir-success') dirSuccess(args, fn)
-      if (nativeBehavior === 'dir-broken') dirBroken(args, fn)
-      if (nativeBehavior === 'dir-error') dirError(args, fn)
-      if (nativeBehavior === 'dir-dest-exists') dirDestExists(args, fn)
-    })
-  })
-
   describe('ensureSymlink()', () => {
     const fn = ensureSymlink
     tests.forEach(test => {
@@ -410,24 +392,6 @@ describe('fse-ensure-symlink', () => {
     })
   })
 
-  describe('fs.symlinkSync()', () => {
-    const fn = fs.symlinkSync
-    tests.forEach(test => {
-      const args = test[0].slice(0)
-      const nativeBehavior = test[1]
-      // const newBehavior = test[2]
-      if (nativeBehavior === 'file-success') fileSuccessSync(args, fn)
-      if (nativeBehavior === 'file-broken') fileBrokenSync(args, fn)
-      if (nativeBehavior === 'file-error') fileErrorSync(args, fn)
-      if (nativeBehavior === 'file-dest-exists') fileDestExistsSync(args, fn)
-      args.push('dir')
-      if (nativeBehavior === 'dir-success') dirSuccessSync(args, fn)
-      if (nativeBehavior === 'dir-broken') dirBrokenSync(args, fn)
-      if (nativeBehavior === 'dir-error') dirErrorSync(args, fn)
-      if (nativeBehavior === 'dir-dest-exists') dirDestExistsSync(args, fn)
-    })
-  })
-
   describe('ensureSymlinkSync()', () => {
     const fn = ensureSymlinkSync
     tests.forEach(test => {

@manidlou @JPeer264 Thoughts here? Should we be testing Node's behavior here, or should we remove these tests? Should we get to the bottom of why it behaves differently here?

@JPeer264

Copy link
Copy Markdown
Collaborator

I am usually a very curious type of person and want to know why something is failing (or working :) ). But actually this is not the job of fs-extra to test against the node behavior, so for the PR itself I would just skip/remove these tests here. If there is time we could go deeper into this, but don't think it is necessary.

@RyanZim

RyanZim commented Apr 24, 2021

Copy link
Copy Markdown
Collaborator Author

Removed the tests

@RyanZim
RyanZim marked this pull request as ready for review April 24, 2021 18:25

@manidlou manidlou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RyanZim
RyanZim merged commit 21b01f4 into master May 1, 2021
@RyanZim
RyanZim deleted the ryan/gh-actions branch May 1, 2021 14:52
@RyanZim RyanZim mentioned this pull request May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants