-
Notifications
You must be signed in to change notification settings - Fork 93
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
Update dependencies to latest versions #198
Update dependencies to latest versions #198
Conversation
I get the same failing tests when running the tests on the master branch on my local computer (i.e. without my changes), so I am not convinced that they origin from this PR. My best guess is that they origin from (transitive) dependency updates since |
Thank you @matsev for your PR. Unfortunately we cannot simply update the dependencies like that because we are still supporting Node v6. |
@raszi Thanks for the feedback. May I suggest that you bump the Node version to v10 which is the current Active LTS release (or at least to Node version v8 which has Maintenance LTS status). v6.9.0 was declared end of live in April this year. Please see the releases page for more information about the Node release schedule. |
@raszi since a few releases back we only support node >=7. @matsev I have checked out your PR and ran the tests locally. It seems as if this is a race condition. After having run the tests locally against node v12.10.0, I encountered the same issue. However, checking the temporary path tells me that the temporary directory had been removed as expected. Yet, during test evaluation, the temporary directory still exists. I will look into this, maybe we need to introduce some, yikes, wait time before checking whether the directory does no longer exist. @matsev It is definitely not a race condition. It seems that with v12.10.0 the behaviour changed and graceful cleanup will no longer work. |
And this happens when you swallow up exceptions, my bad:
|
So we have multiple errors here:
|
It seems that the behaviour of node fs changed in at least 12.0.x. With the introduction of the experimental option After having changed the rmdirsync callback to rimraf the tests will no longer fail and the empty directories will be removed as expected. node 12.10.x seems to be broken. I am still figuring out on how we could achieve this without reimplementation of recursive deletes. |
The actual failure lies in node v12.10.x which introduced the, seemingly, non optional second argument to fs.rmdirSync, which expects it to be a configuration object. And while we definitely should fix our software, it stands to reason on why the peops over at node would introduce an optional, yet mandatory, formal parameter... while they could always check for additional parameters via the arguments list. |
In addition, it seems as if node v12.10.x is now preventing from removing empty directories if the experimental recursive option is missing, again, something that we would refrain on since we need the user to be sure that recursively deleting non empty directories will result in an expected failure. |
@matsev I have some changes that I would like to include with your PR. Can you please check the appropriate option for the PR? See https://help.github.com/en/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork for more information. |
@silkentrance I guess that you are referring to the Allow edits from maintainers checkbox? As far as I can tell it is checked so you should be good to go. |
ed47537
to
14fc96a
Compare
@matsev I have rebased you PR to the latest master. Let's see whether this will build without errors. |
@matsev build is still failing with latest version of node. need to investigate. |
@matsev I somehow have misplaced the fix I made for this issue as mentioned in #199 (comment). Now, which machine has it... |
…nt to fs.rmdirSync
Finally I managed to get my head around this. Turned out that I had to actually provide my credentials when pushing to your branch. In the past I thought this to be a mistake of mine. Damn. |
Working now. Merging now. |
The
rimraf
dependency is out of date, and so are some of thedevDependencies
. This PR updates the dependencies to the latest version.