Feature/rename

[DaQuirm]

This adds rename/renameSync as described in:

http://man7.org/linux/man-pages/man2/rename.2.html

[AviVahl]

Awesome work. I'm aware changing the base APIs requires a bit more work than the extended ones.
I hope it wasn't too painful having to find/edit all these integration points (I believe you covered them all, so kudos!).

Also nice to see that you've found your way around the memory fs! Great work there.

Several points that came to mind during the review:

• describe could be separated into "renaming files" and "renaming directories", to be more consistent with other describes. Directory describe would probably contain more tests, due to behaviors with existing target directory (empty or not).
• tests currently use sourcePath. using sourceFilePath / sourceDirectoryPath might make following what's being renamed easier. though if describes are separated, this point might be redundant.
• after sync tests are reviewed/refactored/ready, please create the async base contract version as well. anyone creating an async-only fs (remote one, for example) should have his rename implementation tested too.
• should memory's rename touch mtime/birthtime?
• see additional comments next to relevant source lines

[AviVahl]

the test description is a bit confusing. we don't usually test that stuff don't happen. the "not throwing" aspect is covered by many tests calling renameSync on various fixtures. If those calls threw, those tests would have failed. Checking it separately feels unneeded.

the expect in this test is covered by the 'moves a directory' test, isn't it?

[DaQuirm]

This testing case defines the desired behavior, other tests may be changed, refactored or even removed, I feel it's good to be explicit about these things but I also understand how it might seem odd. The best way I'd say is to express such things with types whenever possible.

[DaQuirm]

First of all, that's a very thorough and insightful review, good job! 👍

• describe could be separated into "renaming files" and "renaming directories", to be more consistent with other describes. Directory describe would probably contain more tests, due to behaviors with existing target directory (empty or not).

I'd suggest using a separate describe for the directories, since a fair share of tests verifies behavior common for both files and directories (e.g. that the existence of containing directories is checked).

Concerning directory copying: a few relevant cases could not be tested adequately without writing platform-dependent tests. The reason is that there are still discrepancies in the way nodejs rename works on different platforms. Consider this open issue for instance: nodejs/node#21957

• tests currently use sourcePath. using sourceFilePath / sourceDirectoryPath might make following what's being renamed easier. though if describes are separated, this point might be redundant.

That's a good point, it makes things clearer! 👍

• after sync tests are reviewed/refactored/ready, please create the async base contract version as well. anyone creating an async-only fs (remote one, for example) should have his rename implementation tested too.

Oh, I didn't think about that option! What do you think about making the test code polymorphic in respect of whether it's run synchronously or asynchronously?

• should memory's rename touch mtime/birthtime?

It definitely should, totally missed that!

• see additional comments next to relevant source lines

Will do!

[AviVahl]

Fixes look good. Please check why tests are failing.

I'm not sure how I feel about the new .sequence() matcher. It feels a bit duplicate. We could rename lastEvent(), and change it to accept either a single event or array of events:

Nice pattern for this:

public async validateEvents(expectedEvents: IWatchEvent | IWatchEvent[]): Promise<void> {
    const { capturedEvents } = this
    if (!Array.isArray(expectedEvents)) {
        expectedEvents = [ expectedEvents ]
    }
    // typescript knows expectedEvents is an IWatchEvent[] at this point in flow
    ...
}

timeout might need to be adjusted (multiplied by expectedEvents.length) and so on.

Regarding the non-consistent cross-platform behavior:

• If all platforms throw an error, but different codes, avoid checking it for the specific code. just verify it throws.
• we could add a boolean flag to the contract test input to mark specific platforms/fs which we know are diverging. That flag can be used to skip problematic tests.
• If some behavior is too problematic to test cross-platform, skip that test and put a comment explaining why.

Other than that, once you finish cleaning up and everything's green, let me know and I'll squash+release.

[DaQuirm]

Okay, watch events proved to be too different to test without resorting to platform-aware code so I'm reverting the test and the changes to WatchEventsValidator. This should make the builds green again 💚

[AviVahl]

Great work!