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

Mocha@3 support #216

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Mocha@3 support #216

wants to merge 7 commits into from

Conversation

a13o
Copy link

@a13o a13o commented Sep 9, 2018

Based on the discussion in #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
Copy link
Author

a13o commented Sep 9, 2018

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".

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2018

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

@rwjblue
Copy link
Member

rwjblue commented Sep 9, 2018

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...

@a13o
Copy link
Author

a13o commented Sep 9, 2018

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/test-helpers@0.7.18 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.

@Turbo87
Copy link
Member

Turbo87 commented Sep 19, 2018

@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

@a13o
Copy link
Author

a13o commented Sep 19, 2018

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

@apellerano-pw
Copy link

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

@Turbo87
Copy link
Member

Turbo87 commented Oct 12, 2018

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

@a13o
Copy link
Author

a13o commented Oct 20, 2018

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

@a13o
Copy link
Author

a13o commented Nov 12, 2018

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 😛

@Turbo87
Copy link
Member

Turbo87 commented Nov 13, 2018

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... 😞

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