Skip to content
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

Implement GH actions #881

Merged
merged 4 commits into from
May 1, 2021
Merged

Implement GH actions #881

merged 4 commits into from
May 1, 2021

Conversation

RyanZim
Copy link
Collaborator

@RyanZim RyanZim commented Apr 1, 2021

No description provided.

@JPeer264 JPeer264 mentioned this pull request Apr 9, 2021
@RyanZim
Copy link
Collaborator Author

RyanZim commented Apr 21, 2021

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

@RyanZim
Copy link
Collaborator Author

RyanZim commented Apr 22, 2021

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
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
Copy link
Collaborator Author

RyanZim commented Apr 24, 2021

Removed the tests

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

@manidlou manidlou left a comment

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