qunitjs / js-reporters

📋 Common Reporter Interface (CRI) for JavaScript testing frameworks.
MIT License
60 stars 18 forks source link

Normalizing assertions #79

Closed flore77 closed 8 years ago

flore77 commented 8 years ago

I am thinking something like:

var assertion = {
  name: String,
  result/successful:  String (failed || passed)/Boolean (true || false),
  actual: Object,
  expected: Object,
  message: String,
  stack: String
};

Certainly actual, expected, message and stack are mandatory. i am not sure about name and result.

I think name, should be the name of the error, this could be useful, to know if the test failed because of an assertion, or it was the actual code that generated an error.

result could be useful to quickly differentiate between a passed assertion and a failed one.

flore77 commented 8 years ago

@jzaefferer what do you think about it?

jzaefferer commented 8 years ago

What are some example values for name?

result lacks in clarity. Maybe passed or failed?

flore77 commented 8 years ago

result lacks in clarity

The property can be called or result with possible values: passed , failed or it can be called succesful with values: true, false.

For name it could be for an error that happend after a failed assertion AssertionError. And other errors that can be generated by other code, could have a personalized name, if the thrown error has one, if not it could be assigned a generic name, like Error.

And for passed assertion, i don't what value should name have. Maybe undefined?

boneskull commented 8 years ago

Is this in scope? This looks like an "assertion library" thing; not a "reporter" thing.

flore77 commented 8 years ago

Indeed assertions are generated by assertion libraries, but they are reported through the reporting interface and it would be nice if all frameworks would emit the assertions in the same format, so that in reporters would be really easy to display/format/etc. them. An example would be the browserstack integration the part in which the error is formatted.

The error object is quite similar between frameworks, see https://github.com/js-reporters/js-reporters/issues/12#issuecomment-120483356 (scroll down to Error object), so I guess it wouldn't be such a great effort to standardize it.

boneskull commented 8 years ago

OK, I see. A couple notes around this regarding Mocha:

flore77 commented 8 years ago

I don't know offhand if this "third state" is part of the specification

Yes, it is part, we defined it as skipped.

So, a boolean success field may then be inappropriate.

It is a good point, indeed, but I think it could be suitable for passed and failed tests, because this tests have assertions that have been evaluated, on the other hand skipped tests have no assertions so there will be nothing to emit for them.

Also this issue is somehow bound to #31, because what we discuss here will be implemented in the pr, #80, that fixes the aforementioned issue, so in case of a skipped test, the assertions property will be a void array, [].

flore77 commented 8 years ago

So there would be:

var passedAssertion = {
  success: Boolean (true),
  actual: Primitive | Object | null,
  expected: Primitive | Object | null,
  message: String,
  stack: undefined
};

var failedAssertion = {
  success: Boolean (false),
  actual: Primitive | Object | null,
  expected: Primitive | Object | null,
  message: String,
  stack: String
};

Normalized assertions will be contained in the test errors prop and assertions prop.

For the testStart event both will be undefined. For the testEnd event:

What do you think about it? @jzaefferer @boneskull

Also do you think the name property would be useful ?

boneskull commented 8 years ago

Mocha cannot emit a list of assertions. It's possible that popular assertion libs could one day have "adapters" which enable advanced integration, but I doubt this would ever be required to use one.

A list of failed assertions will also never be greater than length one (1), since the first failed assertion will fail the test.

Tangentially, it could be argued a test containing multiple assertions is an anti-pattern. I often make this case, but I'm not convinced that it is applicable to test frameworks in general.

Mocha sacrifices this level of introspection for flexibility. I don't often see users asking for tighter integration with assertion libraries; those that prefer this kind of behavior may be better served by other frameworks.

The name prop is not necessary for the first specification IMO; if it's desired it could be added without breaking backwards compatibility (I'm unconvinced "named" exceptions have a real use case in JavaScript anyhow, but that discussion needn't happen here).

boneskull commented 8 years ago

You may as well call actual/expected "*" since either one may also be undefined.

flore77 commented 8 years ago

Yes, we are aware that Mocha cannot provide this. But this is ok, because this assertions prop is not mandatory, it is for test frameworks like Jasmine and QUnit, that provide a list of passed/failed assertions per test, because they will not bail on the first failed assertion.

What do you recommend for Mocha for this assertions prop to be an empty array or to contain the one error (thrown by the first failed assertion) or just to be undefined?

boneskull commented 8 years ago

What do you recommend for Mocha for this assertions prop to be an empty array or to contain the one error (thrown by the first failed assertion) or just to be undefined?

If I was writing a user-facing API, I'd go for flexibility...

...but to reduce cognitive overhead for implementors of reporters, it may be prudent to simply require an Array value, even if empty. This may be somewhat inconvenient for Mocha--but it absolves the reporter of having to check types, etc.

jzaefferer commented 8 years ago

Defaulting empty array for assertions (for testEnd) sounds good to. I also agree that we can drop the name property from the spec - implementers can still provide additional properties on top.

Regarding skipped, to clarify: That is a status that tests can have, not assertions.

How about passed instead of success?

flore77 commented 8 years ago

I agree with defaulting an empty array.

How about passed instead of success?

Yes, it's ok.

flore77 commented 8 years ago

Implemented in #80.