Attempt to get tests passing with updated deps

[phated]

This probably needs work still but I needed to open a PR to run on AppVeyor.

[coveralls]

Coverage decreased (-0.0008%) to 98.47% when pulling 6ff94cb on phated:replace-syspath into cbdf255 on paulmillr:master.

[coveralls]

Coverage decreased (-0.0008%) to 98.47% when pulling 6ff94cb on phated:replace-syspath into cbdf255 on paulmillr:master.

[phated]

@es128 any idea why this didn't run on AppVeyor?

[es128]

Webhook config might be messed up. I'm not able to look at it.

@paulmillr can you check? The webhook url is
https://ci.appveyor.com/api/github/webhook?id=jvv568xm6xsow034

[phated]

Alright, I'll set up a temp hook on my appveyor account.

[coveralls]

Coverage decreased (-0.0008%) to 98.47% when pulling 1a4ba3d on phated:replace-syspath into cbdf255 on paulmillr:master.

[paulmillr]

just resubmitted to appveyor

[paulmillr]

Please push a new commit @phated

[phated]

@es128 @paulmillr I'm noticing some fundamental problems with the tests for the major bump - mainly that getFixturesPath needs to be modified to always return a posix-style path but it's also used to generate expected paths in many tests. Do you think chokidar should emit posix-style paths in the same fashion as the globs it takes or should the tests be re-written to separate the expectations from the glob paths?

[paulmillr]

Don't think we should break any backwards compatibility.

[phated]

The purpose of the major bump is to break compat with Windows style paths because they aren't valid globs. All dependencies have been upgraded for that behavior

[paulmillr]

Should we do a 2.0 release then?

[phated]

@paulmillr yeah, I believe @es128 suggested the dependency updates to land in 2.0 in his #632 PR - I was just trying to get the tests to pass when I noticed #658 (comment)

[phated]

I'm happy to make the changes but I need the direction to take 😄

[es128]

should the tests be re-written to separate the expectations from the glob paths?

That would be my vote. I don’t think forcing posix-style paths in the events makes sense.

Maybe leave getFixturesPath as it was and a new method for generating the absolute-path globs for watch input?

[phated]

@es128 sure, I can look into that. I'm having a hard time understanding the core impl so it's taking me awhile.

[phated]

@es128 I believe I have all the tests passing now (except maybe 1 flakey Windows polling test). However, I don't know if this is the proper amount of changes for this bump. I had to do some really weird things (that you'll see in the diff) to get these to pass).

cc @doowb

[phated]

Not sure why node 4 failed on AppVeyor - it's passing on mine: https://ci.appveyor.com/project/phated/chokidar/history

[coveralls]

Coverage increased (+0.007%) to 98.477% when pulling a92f089 on phated:replace-syspath into cbdf255 on paulmillr:master.

[phated]

I have no idea what's going on with the Travis runs :(

[es128]

It's not unusual to have flaky failures because of how the test suite thrashes the filesystem, especially on weak CI VMs... especially especially the OS X ones.

This all looks pretty good to me. Can you point out what part of the patch seems weird/objectionable to you? I just took a quick look, but don't have concerns with anything I saw.

[phated]

@es128 I'm specifically concerned with the changes I had to make to .add that does normalization to joined cwd and potentially a glob path. I'm also not comfortable with the codebase but noticed a lot of sysPath usage - which would cause issues if a glob was passed to any of those.

[es128]

The changes to .add make sense to me for the specific reason you described - we'll no longer be able to use isGlob on anything that's been sysPath'd.

If there are any other bad combinations of sysPath + the newly stricter globbing deps I'd expect the tests to tell us.

I'd be comfortable letting this out with a major bump. We could start with a prerelease tag also to try to get early feedback before it goes wide.

[phated]

I'm good with that if you are comfortable with it.

[phated]

Crud, I need to update anymatch too.

[coveralls]

Coverage increased (+0.007%) to 98.477% when pulling 2f3112a on phated:replace-syspath into cbdf255 on paulmillr:master.

[phated]

@es128 can you rerun the CI?

[phated]

@es128 looks like just 1 OSX failure now.

[phated]

woohoo! @es128 do you want appveyor to be green too? I think @paulmillr would have to restart it.

[es128]

No, there’s no point in trying to get the 0.10 one green in appveyor. It fails on something random more often than it succeeds.

[es128]

@paulmillr the appveyor webhook still doesn't seem to be working. After merging I just had to kick it off manually, and the only option exposed to me in their UI is to do that for the latest commit in the master branch.

[phated]

@es128 did you have to manually kick it off at https://ci.appveyor.com/project/es128/chokidar and https://ci.appveyor.com/project/paulmillr/chokidar? It seems the location changed when he re-enabled it

Also, thanks for getting this merged! Excited to see 2.0 so I can get it updated in glob-watcher 😸

[es128]

Oh I hadn't even noticed the divergence. So yeah, I guess the new one at https://ci.appveyor.com/project/paulmillr/chokidar should be good to go.

[phated]

Do you need anything else from me before it gets released? Where do you want to mention the breaking glob changes?

[es128]

@paulmillr do you have any housekeeping items you'd like to see prior to publishing 2.0?

I've been mulling over the idea of opening a PR and a discussion about dropping 0.10 and 0.12 from the CI configs... and maybe announcing an end of active support for them with the major bump, without doing anything to deliberately break compatibility.

[phated]

@es128 What about deprecating them in the major and moving them to allow_failures in the CI configs?

[realityking]

Outside observer dropping in: IMHO either you drop a version or you don't. Soft-deprecations inevitably turn into hard failures when a regression isn't noticed (because they're dropped from CI). That's a major pain for downstream packages who don't expect node version to be (effectively) dropped in a minor release.

[es128]

Hmm, I hadn't really considered the allow_failures option. If I can turn that on in appveyor for node v0.10 that may be good enough. Don't even really need to deprecate compatibility with the old versions, they should just keep working as well as they already do.