Reporter: handle objects with cycles #130

zackthehuman · 2021-02-14T08:41:57Z

Updating TapReporter with the ability to handle objects with circular
references. This is needed for proper stringification of actual and
expected values that contain cycles.

Fixes #104

ljharb · 2021-02-14T15:25:15Z

lib/reporters/TapReporter.js

+ * it will be represented as "[Circular]".
+ */
+function createCycleSafeReplacer() {
+  const seen = new Set();


Suggested change

const seen = new Set();

const seen = new WeakSet();

I made this change but then later didn't need it since I switched to using a shallow clone and the cloning is stack-based.

ljharb · 2021-02-14T15:25:42Z

lib/reporters/TapReporter.js

+
+    if (typeof value === 'object' && value !== null) {
+      if (seen.has(value)) {
+        return '[Circular]';


node’s util.inspect tracks specific refs here, so you know what it’s circular of - it might be worth doing that here.

This is indeed a neat feature of inspect. One question is whether this would be confusing seeing it in the actual/expected output. To label the refs we need two parts:

To label the object being referenced as a <ref *n>

To use the correct index when labeling cycles [Circular *n]

Labeling the cycles should be pretty easy, but I'm a little worried about changing the structure of the stringified object too much.

There are some other enhancements to this stringification we need to consider, for example, BigInt also throws when being fed to JSON.stringify. I'd rather follow up on enhancements separately and landing this fix first.

Thoughts?

Right - normal JSON.stringify isn't really an option, I think, at least not without a stringifier function also provided.

zackthehuman · 2021-02-14T17:05:19Z

I need to crunch on this a little bit more. The existing approach can have false positives as long as the same object appears more than once (even when it's not cyclical). I also have to fix the linting errors.

zackthehuman · 2021-02-20T19:48:15Z

Circling back on this: I think there are many improvements we can make to the serialization outside of handling ref cycles. It would be worth having an issue dedicated to those improvements so that we can figure out what "ideal" serialization looks like.

In the short term, it would help me (and others) to land this fix. I've squashed/rebased my changes, so it should be in a reviewable state. @Krinkle if you have any time would you mind taking a look?

Krinkle · 2021-02-21T01:05:14Z

lib/reporters/TapReporter.js

+      clone = object.map(function (element) {
+        return decycledShallowClone(element, ancestors);
+      });
+      ancestors.pop(object);


Array.pop() does not take an argument.

Krinkle · 2021-02-21T01:06:49Z

lib/reporters/TapReporter.js

+ * been replaced with "[Circular]".
+ */
+function decycledShallowClone (object, ancestors = []) {
+  if (ancestors.includes(object)) {


Use Array#indexOf() !== -1 here for browser compat as I'd rather not ship a polyfill with this library, and given its low-level use in test runners it's not feasible for end-users to ship a polyfil either as they generally can't or wouldn't be expected to load something before this.

Fixed. Should I also avoid Array#map?

Krinkle · 2021-02-21T01:07:02Z

lib/reporters/TapReporter.js

+      Object.keys(object).forEach(function (key) {
+        clone[key] = decycledShallowClone(object[key], ancestors);
+      });
+      ancestors.pop(object);


TIL what idem means.

Krinkle · 2021-02-21T01:07:57Z

test/fixtures/unit.js

+ * than once in an acyclical way.
+ */
+function createDuplicateAcyclical () {
+  const duplicate = {};


To improve code coverage a little bit, and to weed out errors that result in objects rendering as empty without keys, perhaps include at least one key-value pair in here.

Good point. Added keys to each of the objects so we can identify them in the output.

Krinkle · 2021-02-21T01:08:35Z

test/fixtures/unit.js

+ */
+function createCyclical () {
+  const cyclical = {};
+  cyclical.cycle = cyclical;


In order to excercise the stack a bit bit, perhaps add one layer of indirection here, e.g. cyclical = { sub: {} }; cyclical.sub.cycle = cyclical;.

Added an additional structure with a subobject cycle.

Updating `TapReporter` with the ability to handle objects with circular references. This is needed for proper stringification of actual and expected values that contain cycles. Fixes qunitjs#104

ljharb reviewed Feb 14, 2021

View reviewed changes

zackthehuman force-pushed the fix-circular-serialization branch from fc2860a to 3258391 Compare February 20, 2021 19:39

Krinkle self-assigned this Feb 20, 2021

Krinkle requested changes Feb 21, 2021

View reviewed changes

zackthehuman force-pushed the fix-circular-serialization branch from 3258391 to 1a009dd Compare February 21, 2021 03:11

Reporter: handle objects with cycles

8307cc4

Updating `TapReporter` with the ability to handle objects with circular references. This is needed for proper stringification of actual and expected values that contain cycles. Fixes qunitjs#104

zackthehuman force-pushed the fix-circular-serialization branch from 1a009dd to 8307cc4 Compare February 21, 2021 03:16

Krinkle approved these changes Feb 21, 2021

View reviewed changes

Krinkle merged commit c38f467 into qunitjs:main Feb 21, 2021

zackthehuman deleted the fix-circular-serialization branch February 21, 2021 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reporter: handle objects with cycles #130

Reporter: handle objects with cycles #130

zackthehuman commented Feb 14, 2021

ljharb Feb 14, 2021

zackthehuman Feb 14, 2021

ljharb Feb 14, 2021

zackthehuman Feb 14, 2021 •

edited

Loading

ljharb Feb 15, 2021

zackthehuman commented Feb 14, 2021

zackthehuman commented Feb 20, 2021

Krinkle Feb 21, 2021

Krinkle Feb 21, 2021

zackthehuman Feb 21, 2021

Krinkle Feb 21, 2021

zackthehuman Feb 21, 2021

Krinkle Feb 21, 2021

zackthehuman Feb 21, 2021

Krinkle Feb 21, 2021 •

edited

Loading

zackthehuman Feb 21, 2021

Reporter: handle objects with cycles #130

Reporter: handle objects with cycles #130

Conversation

zackthehuman commented Feb 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackthehuman Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackthehuman commented Feb 14, 2021

zackthehuman commented Feb 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krinkle Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackthehuman Feb 14, 2021 •

edited

Loading

Krinkle Feb 21, 2021 •

edited

Loading