search

qunitjs / js-reporters

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

Data enhancement #81

Closed flore77 closed 8 years ago

flore77 commented 8 years ago

While working on https://github.com/js-reporters/js-reporters/tree/update-spec, I noticed that the test name property is called testName, I think it is not necessary to be called this way, if it is already attached to a test object, as also it was not in sync with the suite object who's name property is called name. I hope you agree.

EDIT: I am thinking to fix multiple things in this pr, that's why the questions below:

  1. Should we add an Assertion class ?
  2. What is the toJSON method about defined on the Suite prototype?
flore77 commented 8 years ago

@jzaefferer ping

jzaefferer commented 8 years ago

Changing to name makes sense. Can land this immediately.

As for toJSON, I can't remember. It was introduced here, which doesn't help: https://github.com/js-reporters/js-reporters/commit/1a54b8ef94fa18432e7e10b3381bc7da45a08ab4#diff-0198054f2bb34911d00c2e884ea25e01R60

Let's try to the defineProperties call and see what happens.

flore77 commented 8 years ago

It was introduced here

Yeah, it doesn't help so much, I see that there was also an Error class that was removed. So is a good idea to create an Assertion class ?

flore77 commented 8 years ago

Please review @jzaefferer.

jzaefferer commented 8 years ago

Looks good!