Test end assertions prop #80

Merged
merged 12 commits into from Jul 26, 2016

Conversation

Projects
None yet
2 participants
@flore77
Contributor

flore77 commented Jul 20, 2016

Fixes #31.

@flore77

This comment has been minimized.

Show comment
Hide comment
@flore77

flore77 Jul 20, 2016

Contributor

I think it would be good to finish first #79. So that we'll know what to expect and to add here proper testing.

Contributor

flore77 commented Jul 20, 2016

I think it would be good to finish first #79. So that we'll know what to expect and to add here proper testing.

@flore77 flore77 referenced this pull request Jul 24, 2016

Closed

Normalizing assertions #79

lib/adapters/QUnitAdapter.js
- this.errors.push(details)
+ var assertion = {
+ success: details.result,
+ actual: details.actual || null,

This comment has been minimized.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

This is wrong. Anything falsy is valid here and should not be replaced with null. I'd remove the fallback, to keep whatever the property was.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

This is wrong. Anything falsy is valid here and should not be replaced with null. I'd remove the fallback, to keep whatever the property was.

lib/adapters/QUnitAdapter.js
+ var assertion = {
+ success: details.result,
+ actual: details.actual || null,
+ expected: details.expected || null,

This comment has been minimized.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Same here

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Same here

lib/adapters/QUnitAdapter.js
+ actual: details.actual || null,
+ expected: details.expected || null,
+ message: details.message,
+ stack: details.source || undefined

This comment has been minimized.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

This fallback seems unnecessary

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

This fallback seems unnecessary

This comment has been minimized.

@flore77

flore77 Jul 25, 2016

Contributor

I agree with the ones above, but I think this is necessary, because passed assertion must have the stack undefined, at least this is what we discussed in #79

@flore77

flore77 Jul 25, 2016

Contributor

I agree with the ones above, but I think this is necessary, because passed assertion must have the stack undefined, at least this is what we discussed in #79

This comment has been minimized.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Can you verify that details.source in QUnit is undefined for passed assertions? If it isn't, we should remove that requirement.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Can you verify that details.source in QUnit is undefined for passed assertions? If it isn't, we should remove that requirement.

This comment has been minimized.

@flore77

flore77 Jul 25, 2016

Contributor

Ok, I will check 😃

@flore77

flore77 Jul 25, 2016

Contributor

Ok, I will check 😃

This comment has been minimized.

@flore77

flore77 Jul 25, 2016

Contributor

Yes, it is undefined. More specifically the object emitted on the log callback does not have this prop.

@flore77

flore77 Jul 25, 2016

Contributor

Yes, it is undefined. More specifically the object emitted on the log callback does not have this prop.

This comment has been minimized.

@flore77

flore77 Jul 25, 2016

Contributor

For Jasmine the stack property is an empty string. So, I think undefined works just fine.

@flore77

flore77 Jul 25, 2016

Contributor

For Jasmine the stack property is an empty string. So, I think undefined works just fine.

This comment has been minimized.

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Alright. I probably was thinking of QUnit providing stacks for tests, even when passed...

@jzaefferer

jzaefferer Jul 25, 2016

Contributor

Alright. I probably was thinking of QUnit providing stacks for tests, even when passed...

lib/adapters/MochaAdapter.js
- // Test end.
+ let errors = []
+
+ // Mocha has indeed only one error per test, but by this is avoided the

This comment has been minimized.

@jzaefferer

jzaefferer Jul 26, 2016

Contributor

There's something wrong with the comment here. "but by this is"? Not sure what you're trying to say.

@jzaefferer

jzaefferer Jul 26, 2016

Contributor

There's something wrong with the comment here. "but by this is"? Not sure what you're trying to say.

This comment has been minimized.

@flore77

flore77 Jul 26, 2016

Contributor

I wanted to make clear why I used forEach, although that Mocha has only one error. I will delete I think.

@flore77

flore77 Jul 26, 2016

Contributor

I wanted to make clear why I used forEach, although that Mocha has only one error. I will delete I think.

This comment has been minimized.

@jzaefferer

jzaefferer Jul 26, 2016

Contributor

Okay

@jzaefferer

jzaefferer Jul 26, 2016

Contributor

Okay

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 26, 2016

Contributor

Looks good to me. If we land this as-is, the next step needs to be a spec update. Add the Error type next to Suite and Test.

Contributor

jzaefferer commented Jul 26, 2016

Looks good to me. If we land this as-is, the next step needs to be a spec update. Add the Error type next to Suite and Test.

@flore77

This comment has been minimized.

Show comment
Hide comment
@flore77

flore77 Jul 26, 2016

Contributor

Hmm, Error type or better Assertion type?

Contributor

flore77 commented Jul 26, 2016

Hmm, Error type or better Assertion type?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 26, 2016

Contributor

You're right, should be Assertion.

Contributor

jzaefferer commented Jul 26, 2016

You're right, should be Assertion.

@flore77 flore77 merged commit 38b07f4 into master Jul 26, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@flore77 flore77 deleted the test-end-assertions-prop branch Jul 26, 2016

@flore77 flore77 referenced this pull request Aug 3, 2016

Merged

Update spec #84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment