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

Make spy properties non-enumerable #1936

Closed
ehmicky opened this issue Oct 25, 2018 · 12 comments
Closed

Make spy properties non-enumerable #1936

ehmicky opened this issue Oct 25, 2018 · 12 comments

Comments

@ehmicky
Copy link
Contributor

ehmicky commented Oct 25, 2018

Is your feature request related to a problem? Please describe.
When a Sinon spy assertions fails during unit testing, some assertion libraries like power-assert (used by Ava) will print all the spy properties (including the spy methods) which is very verbose and makes it harder to see why the test failed.

Describe the solution you'd like
Make Sinon-specific spy properties non-enumerable.

Describe alternatives you've considered

  • customizing the assertion library power-assert but it's hard to do from Ava
  • customizing Ava but it's not possible because it directly relies on power-assert for this case
  • overriding spy[Symbol.toStringTag] or spy.toJSON() but this does not work since power-assert does not use either. Instead it iterates over enumerable properties.

Additional context
The feature request is about spies, but it most likely can be related to other Sinon objects, such as stubs and fakes.

Here's a screenshot of how verbose a single test Sinon assertion can become with Ava:

enumerable_properties

@fatso83
Copy link
Contributor

fatso83 commented Oct 26, 2018

How would this affect REPL usage and API discoverability? One of my favorite ways of using new tech is through exploratory testing on the REPL. Doing var spy = sinon.spy(); spy. and then tab-ing my way through the various methods. Would this still work?

@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 26, 2018

Yes, non-enumerable properties are still shown on tab autocomplete in Node.js REPL.

@mroderick
Copy link
Member

I think this would be a good improvement, and can't currently think of any downsides.

@mroderick
Copy link
Member

mroderick commented Oct 31, 2018

This has been implemented with #1941 and released as sinon@7.1.1

@ehmicky
Copy link
Contributor Author

ehmicky commented Nov 27, 2018

Thanks for this!

However I do not think this worked:

> sinon.spy()
{ [Function: proxy]
  isSinonProxy: true,
  called: false,
  notCalled: true,
  calledOnce: false,
  calledTwice: false,
  calledThrice: false,
  callCount: 0,
  firstCall: null,
  secondCall: null,
  thirdCall: null,
  lastCall: null,
  args: [],
  returnValues: [],
  thisValues: [],
  exceptions: [],
  callIds: [],
  errorsWithCallStack: [],
  displayName: 'spy',
  toString: [Function: toString],
  instantiateFake: [Function: create],
  id: 'spy#0' }

> Object.keys(sinon.spy())
[ 'isSinonProxy',
  'called',
  'notCalled',
  'calledOnce',
  'calledTwice',
  'calledThrice',
  'callCount',
  'firstCall',
  'secondCall',
  'thirdCall',
  'lastCall',
  'args',
  'returnValues',
  'thisValues',
  'exceptions',
  'callIds',
  'errorsWithCallStack',
  'displayName',
  'toString',
  'instantiateFake',
  'id' ]

Those keys are still enumerable.

As a consequence the original problem still arises.

@mroderick
Copy link
Member

@ehmicky which version of sinon are you using to produce that output?

@mroderick mroderick reopened this Dec 8, 2018
@ehmicky
Copy link
Contributor Author

ehmicky commented Dec 8, 2018

I am using Sinon 7.1.1, Node 11.3.0, Ubuntu 18.10.

@stale
Copy link

stale bot commented Feb 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 6, 2019
@ehmicky
Copy link
Contributor Author

ehmicky commented Feb 6, 2019

Bumping because this issue is still valid with Sinon 7.2.3, Node 11.9.0, Ubuntu 18.10.

@stale stale bot removed the stale label Feb 6, 2019
@fatso83
Copy link
Contributor

fatso83 commented Feb 7, 2019

Ah, @LouisBrunner explicitly just made the functions non-enurable. The other non-function properties are still enumerable. Don't you still want to see those props?

@fatso83
Copy link
Contributor

fatso83 commented Feb 7, 2019

To make the props non-enurable, as well, you would have to use Object.defineProperties on the list of props that are created here.

Seems quite doable. Feel up to it, @ehmicky? A lot more likely it will be done if you are scratching your own itch :-)

@ehmicky
Copy link
Contributor Author

ehmicky commented Feb 7, 2019

Here you go: #1975

Ended up being more than one place to modify.

@mantoni mantoni closed this as completed Feb 25, 2019
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

No branches or pull requests

4 participants