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

Rewrite copySync #519

Merged
merged 5 commits into from
Nov 19, 2017
Merged

Rewrite copySync #519

merged 5 commits into from
Nov 19, 2017

Conversation

manidlou
Copy link
Collaborator

This is the rewrite of copySync to match copy. It should resolve #83, #198, and #464. Added corresponding tests for those issues. Also, removed the fixtures folder that was used by the copy-sync-preserve-time.test.js, instead the fixtures directory is created in the code.

Please let me know if anything doesn't look alright!

@manidlou
Copy link
Collaborator Author

IDK! it is so strange! the unit test for the case when preserveTimestamp is true, sometimes pass and sometimes doesn't! particularly on older node versions! 😕

@manidlou
Copy link
Collaborator Author

Any ideas?

@coveralls
Copy link

coveralls commented Nov 11, 2017

Coverage Status

Coverage decreased (-0.9%) to 85.241% when pulling 5d1f2d3 on rewrite-copySync into 03fba97 on develop.

Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Repository owner deleted a comment from coveralls Nov 11, 2017
Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Looks good

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 11, 2017

Will look at timestamps later.

@RyanZim RyanZim self-assigned this Nov 14, 2017
@RyanZim
Copy link
Collaborator

RyanZim commented Nov 15, 2017

@manidlou Spent a bunch of time messing with this, can't figure it out. I'm gonna say just put a conditional in place that that flaky test gets skipped on older node versions. @jprichardson Let me know if you disagree here.

@manidlou
Copy link
Collaborator Author

@manidlou Spent a bunch of time messing with this, can't figure it out. I'm gonna say just put a conditional in place that that flaky test gets skipped on older node versions.

I agree. I played with it too, but no success! I am not sure but it seems like it is somehow related to node internals.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 85.364% when pulling 5bdbdc4 on rewrite-copySync into 03fba97 on develop.

@manidlou
Copy link
Collaborator Author

If looks good, I will squash this.

@manidlou
Copy link
Collaborator Author

@jprichardson do you have any concerns regarding this?

@jprichardson jprichardson merged commit 86d2ad6 into develop Nov 19, 2017
@jprichardson jprichardson deleted the rewrite-copySync branch November 19, 2017 06:17
@jprichardson
Copy link
Owner

Nice work @manidlou!

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.

4 participants