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

Improved the implementation of expectAssertion #6

Merged
merged 1 commit into from
Mar 12, 2017

Conversation

workmanw
Copy link
Owner

This is to handle assertions thrown inside of a runloop during an integration test. Related to: emberjs/ember.js#14898 .

… thrown inside of a runloop during an integration test. Related to: emberjs/ember.js#14898 .
@workmanw workmanw merged commit b72dbd8 into master Mar 12, 2017
@workmanw workmanw deleted the expectassertion-ember-runloop branch March 12, 2017 17:15
@rwjblue
Copy link
Collaborator

rwjblue commented Mar 12, 2017

I have thoughts...

I'd prefer to avoid clobbering Ember.Test.adapter, lets make ember-qunit expose an override to make this a bit simpler?

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 12, 2017

so we could do:

import { setAdapterExceptionHandler } from 'ember-qunit';

// ... snip ...

QUnit.assert.expectAssertion = function() {
   let lastError;
   setAdapterExceptionHandler((error) => lastError = error);
   // snip rest of code
};

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 12, 2017

@workmanw - Thoughts?

@workmanw
Copy link
Owner Author

workmanw commented Mar 12, 2017

@rwjblue Yea, that works for me. We also need a way to remove or reset that exception handlers? I'd be happy to submit a PR to ember-qunit.

EDIT: Actually I guess we could just do setAdapterExceptionHandler(null); to reset it.

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 12, 2017

I was thinking null for reset, yes

@workmanw
Copy link
Owner Author

@rwjblue Do you have any objection to me cutting a release with this PR. Then immediately working on setAdapterExceptionHandler? I'm looking to provide relief for the issue discussed in: emberjs/ember.js#14898

@rwjblue
Copy link
Collaborator

rwjblue commented Mar 12, 2017

nope, no objections. The thing I'm suggesting above is really just another iteration forward...

@workmanw
Copy link
Owner Author

Excellent. Thanks for the feedback and direction. I'll submit PRs to make this happen.

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.

2 participants