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

Expose the existing mechanisms behind assert.deepEqual #33738

Closed
osher opened this issue Jun 5, 2020 · 10 comments
Closed

Expose the existing mechanisms behind assert.deepEqual #33738

osher opened this issue Jun 5, 2020 · 10 comments
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. stale

Comments

@osher
Copy link
Contributor

osher commented Jun 5, 2020

Is your feature request related to a problem? Please describe.

I've been working on a testing tools set as part of some new proprietary ecosystem
(with the intent to open-source elements from it once they are stable enough in their form and require less maintenance, but so far it's proprietary :( ).
As part of this testing framework, I need to perform a deep comparison of objects.

As a start - I got assert.deepEqual - which is great. However - it ends with an exception.
Sometimes I would like to manage the exception myself - I just need the indication.

I decided to open this issue when I came across more use-cases:

  • data healer - need to act on a decision if a model does not comply with expected values.
    For this - we need a form of containsEql.
  • suite setup & teardown - need to add test-fixture data, and validate that essential objects are part of the target env (e.g. object describing super-user, or root categories, root tags, etc).

In all of the cases, it's beneficial to be able to perform both containsEql and deepEqual.

I did see util.isDeepStrictEqual - and despite the confusing name that gave me hope.

(I mean, it took me a lot of tries to understand that strict does not mean reference-comparison, but comparison without conversion of values. The first time I saw it I struggled to understand what it means - does it compare shallow copies? I had to try and see for myself. Anyway - I'm side tracking)

Describe the solution you'd like
I would propose on the same spirit of util.isDeepStrictEqual(..)
util.isDeepStrictContain(..)

I also saw the underlying mechanisms with the bStirct flag, and thought it could also be useful to let users inject it if they so choose, but I'm not sure about the names or the form it should take.

I'll be happy to start work on it if we can agree on the form it should take.

I mean, the mechanisms are there, and their functionality is all well-tested.
We just need to feature them and add only tests that show they are available, or even reuse parts of the existing tests - but expect a boolean instead of an error.

Describe alternatives you've considered
All current alternatives come from userland codes, which basically duplicate the same logic and introduce their own takes and flavors. Which is a pity...
We have that logic in node, and it's a good logic that runs deep and already serves now a role in APIs used by userland.

@BridgeAR BridgeAR added assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. labels Jun 5, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jun 5, 2020

The strict in the utility function is indeed unfortunate.

I am fine to expand the functionality in a similar fashion as laid out here. I would like to have options to define specific behaviour such as abstractEquality and prototypeCheck. To do so, we should add a new function without the strict in the name.

Adding the contains functionality is a tiny bit more work and we should make sure the performance won't degrade in case we combine the logic.

@benjamingr
Copy link
Member

As a start - I got assert.deepEqual - which is great. However - it ends with an exception.
Sometimes I would like to manage the exception myself - I just need the indication.

Can you explain why (other than API ugliness) you can't accomplish this by wrapping the assert with a try/catch and seeing if an exception was thrown?

util.isDeepStrictContains(..)

That sounds reasonable to me, but I'd like to understand how that behaves - and if we add an isDeepStrict... variant we should also add an assert variant most likely.

@osher
Copy link
Contributor Author

osher commented Jun 7, 2020

@benjamingr

wrapping the assert with a try/catch and seeing if an exception was thrown?

Sure. Try-Catch is a heavy mechanism. The construction of an error is heavy and throwing and catching it is heavy. Transpiled async wraps it in Promise.reject is lighter than throw - but it still goes through the constructor and the stack-trace gathering.
More of then than not I don't mind that in tests - but I do mind that on production code.
(how purist of me... but yes)

we should also add an assert variant most likely

Hmm. That makes sense to me.
My base assumption is that we're using existing logic, no new wheels. We'll have to start and see where does this road go

It's a dangerous business, Frodo, going out your door. You step onto the road, and if you don't keep your feet, there's no knowing where you might be swept off to.

😉

@BridgeAR
I'd love to do a performance bench, we have to get a starting point of reference to measure the change.
What I usually do is open the node shell and write snippets based on Date.now or even process.hrtime if the difference is not that evident.
see example here.
However, in many cases it's hard to get an overwhelming conclusion, and harder test it manually across OS.

How would you propose to test performance?
Is there a way such a test is found in this codebase you can point me to as a reference?

@BridgeAR
Copy link
Member

BridgeAR commented Jun 7, 2020

There are enough benchmarks in ./benchmark/assert/*. It's possible to compare before and after by running something like: node benchmark/compare.js --old ./nodeOld --new ./nodeNew assert > assert.csv and afterwards cat assert.csv | Rscript benchmark/compare.R.

@osher
Copy link
Contributor Author

osher commented Jun 8, 2020

@BridgeAR - that's great. I found the benches.
did not quite understand the compare command. what should the --old and --new parameters provide? path to a node compilation? saved snap of the js sources?

MM. I also understand I have to install R runetime or something?

@BridgeAR
Copy link
Member

BridgeAR commented Jun 8, 2020

@osher the parameters are documented in benchmark/compare.js. The mentioned ones are binaries and it allows to compare different Node.js versions.

R is used to transform the raw data in nice to read output such as x percent increase / decrease between different benchmarks.

@lundibundi
Copy link
Member

@osher take a look at this guide for detailed explanation https://github.com/nodejs/node/blob/master/doc/guides/writing-and-running-benchmarks.md.

@osher
Copy link
Contributor Author

osher commented Jun 8, 2020

awsome. cool stuff.
just what I was looking for!

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 8, 2022
@targos targos moved this to Pending Triage in Node.js feature requests Mar 8, 2022
@targos targos moved this from Pending Triage to Stale in Node.js feature requests Mar 8, 2022
@benjamingr
Copy link
Member

Yeah this has stalled and can still be explored sometime in the future if people put the time/effort :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. stale
Projects
None yet
Development

No branches or pull requests

4 participants