ember-mocha icon indicating copy to clipboard operation
ember-mocha copied to clipboard

Mocha@3 support

Open a13o opened this issue 6 years ago β€’ 11 comments

Based on the discussion in https://github.com/emberjs/ember-mocha/issues/206 this PR adds support for mocha v3.5.3.

The behavior of .only was modified to match the new behavior added in mocha 3.

Mocha 3 also introduces an overspecification exception if your test both returns a promise and uses the done hook. I removed this exception so overspecified tests will still work. It accomplishes this by effectively making every test async and ignoring the done hook.

a13o avatar Sep 09 '18 07:09 a13o

I want to update the yarn.lock (even though the README says to use npm) but I keep getting this error. Anyone know how to get around this? I don't use yarn much.

yarn install v1.9.4
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
error An unexpected error occurred: "http://ynpm-registry.corp.yahoo.com/@ember/test-helpers/-/test-helpers-0.7.18.tgz: connect ECONNREFUSED 68.142.238.39:80".

a13o avatar Sep 09 '18 16:09 a13o

Likely the yarn lock accidentally checked in referring to a private registry (note the yahoo.com there).

rwjblue avatar Sep 09 '18 16:09 rwjblue

I’m unsure how to force yarn to use a registry that isn’t what the yarn.lock has without just blowing away the yarn.lock...

rwjblue avatar Sep 09 '18 17:09 rwjblue

Ahh, I see. I know it says not to at the top... but I edited yarn.lock to replace the yahoo registry entry with the one you get doing yarn add @ember/[email protected] in a new project. Then I ran yarn. Seems like the diff makes sense. Only packages related to the mocha upgrade got bumped in the lockfile.

a13o avatar Sep 09 '18 17:09 a13o

@a13o did you test these changes with a few projects? e.g. https://github.com/TryGhost/Ghost-Admin is a good open source project to test against

Turbo87 avatar Sep 19 '18 07:09 Turbo87

No, I'll start testing this with other projects using ember-mocha and report back. It's gonna take me some time to get to as my upcoming weekend is already full

a13o avatar Sep 19 '18 14:09 a13o

I ran this branch today on a private repo with 6000+ tests across an app and an addon. I ran it on the suite both before and after we did the new testing api upgrade. Worked in all cases!

Let's keep documenting successes so we can build confidence in the PR or fix any issues found

apellerano-pw avatar Oct 11 '18 18:10 apellerano-pw

that's good to hear, thanks @apellerano-pw!

Turbo87 avatar Oct 12 '18 11:10 Turbo87

Tested this on Ghost-Admin, it seems to break in tests with done hooks. Needs further investigation

a13o avatar Oct 20 '18 18:10 a13o

I think my approach to squelching the overspecification error is wrong. It might have to go in the other direction and modify the wrapper function in the adapter to always return a promise, rather than always use a done hook. The done hook would then never actually be given to mocha and can be re-implemented in the adapter to resolve the promise that mocha was given. That means mocha never sees both a returned promise and done hook in the same test, and the error is squelched. And we can detect the arity of the test function before executing it whereas we have to execute it to find out if it returns a promise. So standardizing on the promise seems like the way to go.

It's gonna take a bigger rewrite than I thought. My time is limited to work on this but I'll keep pushing when I can and leaving updates here. The next step is I believe I can write test cases now for what we need.

I'm not sure what the long term plans are for this project; has the possibility of a major version bump and breaking change been discussed? If so I would rather yield to that than continue this approach of removing official mocha functionality in the name of backwards compatibility. Every day we stray further from mocha's light πŸ˜›

a13o avatar Nov 12 '18 04:11 a13o

has the possibility of a major version bump and breaking change been discussed? If so I would rather yield to that than continue this approach of removing official mocha functionality in the name of backwards compatibility. Every day we stray further from mocha's light πŸ˜›

yeah, it had been discussed. but back then the new testing API was still very young and we decided to keep support for the old one for now. the new API has been around for some time now though, so maybe it's time to deprecate the old API. on the other hand ember-cli-mocha is still not updated to the new API yet... 😞

Turbo87 avatar Nov 13 '18 19:11 Turbo87