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

Data enhancement #81

Merged
merged 4 commits into from
Aug 1, 2016
Merged

Data enhancement #81

merged 4 commits into from
Aug 1, 2016

Conversation

flore77
Copy link
Contributor

@flore77 flore77 commented Jul 27, 2016

No description provided.

@flore77
Copy link
Contributor Author

flore77 commented Jul 27, 2016

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
Copy link
Contributor Author

flore77 commented Jul 27, 2016

@jzaefferer ping

@jzaefferer
Copy link
Member

Changing to name makes sense. Can land this immediately.

As for toJSON, I can't remember. It was introduced here, which doesn't help: 1a54b8e#diff-0198054f2bb34911d00c2e884ea25e01R60

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

@flore77
Copy link
Contributor Author

flore77 commented Jul 30, 2016

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
Copy link
Contributor Author

flore77 commented Jul 30, 2016

Please review @jzaefferer.

@jzaefferer
Copy link
Member

Looks good!

@flore77 flore77 merged commit 31a233e into master Aug 1, 2016
@flore77 flore77 deleted the refactor-data branch August 1, 2016 18:53
@flore77 flore77 mentioned this pull request Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants