nodejs-should is extremely commonly used in unit tests for Node.js modules.[[BR]] https://bugzilla.redhat.com/show_bug.cgi?id=911188
However, it has adapted code from Node.js's assert module (which is part of Node.js itself), specifically the _deepEqual routine (only around 100 lines from assert.js).
The routine has been modified for it's own use and changes its behaviour. I'm not sure it's plausible to patch these modules to use the upstream _deepEqual while simultaneously preserving functionality that authors of other modules will expect when using nodejs-should as part of unit tests where consistency is important.
The nodejs-should authors say:[[BR]] "[Our version is] taken from node's assert module, because [node's assert module] sucks and exposes next to nothing useful."
In fact, nodejs-deep-equal (which has passed review) contains nothing more than a forked _deepEqual routine and provides it as a library: https://bugzilla.redhat.com/show_bug.cgi?id=915082
The nodejs-vows module (currently going through review) also has their own version of the _deepEqual routine: https://bugzilla.redhat.com/show_bug.cgi?id=911229
I would therefore like to request a bundling exception for nodejs-should and would appreciate any advice.
T.C. Hollingsworth (patches) who is owner of the F19 Node.js feature had this to say:
T.C. Hollingsworth wrote:
Many test frameworks "reinvent the wheel" when it comes to the assert module in the standard library because exceptions in Node are slooooowwww, and suck for other reasons as well. (Google the subject; there are tons of blog posts about it ;-) I really don't think forking code like this a Big Deal, since it's a critical part of test frameworks.
Many test frameworks "reinvent the wheel" when it comes to the assert module in the standard library because exceptions in Node are slooooowwww, and suck for other reasons as well. (Google the subject; there are tons of blog posts about it ;-)
I really don't think forking code like this a Big Deal, since it's a critical part of test frameworks.
Tom Hughes (tomh) posted about this on fedora-packaging ML:[[BR]] https://lists.fedoraproject.org/pipermail/packaging/2013-March/008912.html
This was Toshio's reply:[[BR]] https://lists.fedoraproject.org/pipermail/packaging/2013-March/008913.html
Following further discussion with toshio, I had another look at trying to use nodejs-deep-equal instead of the bundled version.
One difference in the bundled copy of the _deepEqual routine is the use of the strict equality operator (===) instead of doing type conversion (==) before comparison. When other modules use nodejs-should for unit tests, it is documented that testing equivalence is always strict. This is the widely expected behaviour of nodejs-should. Upstream have not yet replied to my request for comment, but it is extremely unlikely that they will change this behaviour.
Since nodejs-deep-equal doesn't do what nodejs-should really wants it to do, they'd be better off using their own version of the _deepEqual routine, and well, that's exactly what they've done.
If patching nodejs-should to use nodejs-deep-equal, it fails its own unit tests because type conversion happens when it shouldn't. One can ignore the test failures, but this would mean other modules using nodejs-should may pass their tests when they aren't meant to. And since testing equivalence one of the main purposes of nodejs-should, it becomes rather less useful when the behaviour is different from what users and other modules expect.
For the reasons above I am very keen on getting a bundling exception for this modified "code fragment". Unless of course somebody tells me I'm wrong :)
Since I'm not going to be here for a few weeks:
I'm +1 for an exception because I do recall that we had a positive vote about allowing code fragments in. I'd be okay with whatever implementation (needs Virtual Provides, does not need virtual provides, etc) that the other FPC members want to do.
I'd love for the code fragments clause to be written into the guidelines (or point it out to me if it's already there). Feel free to ping me if someone drafts it up and I need to vote on that in a ticket in the next two weeks.
Bundling exception for nodejs-should to include the forked code fragment from the Node.js "assert" module is approved (+1:7, 0:0, -1:0)
Metadata Update from @toshio: - Issue assigned to spot
Login to comment on this ticket.