Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Object comparison is broken for wrapped native and frozen objects #266

Closed
kevinoid opened this Issue · 15 comments

3 participants

Kevin Locke Davis W. Frank James Ide
Kevin Locke

The jasmine.Env.prototype.compareObjects_ function temporarily adds __Jasmine_been_here_before__ to both objects being compared. This fails silently for ES5 frozen objects, causing the method to recurse infinitely for cyclic objects, and fails by throwing an NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN for all WrappedNative objects in XULRunner applications (such as Firefox/Thunderbird extensions). Please consider replacing the implementation with one that does not modify the objects under comparison (e.g. a stack-based cycle detection algorithm such as the Underscore.js equals implementation).

Davis W. Frank
Owner

Thanks for finding this and letting us know about this case. I've logged this bug in Tracker: https://www.pivotaltracker.com/story/show/34261165

Thanks for using Jamsine!

James Ide

I have a patch that use ES6 WeakMaps to keep track of the visited objects. Is this something you'd be interested in? (I plan to use node --harmony but a simple polyfill should suffice since I don't rely on weak references.) cc @gmoeck

Davis W. Frank
Owner

At this time we don't want to depend on ES6 or a polyfill that's forward compatible. After looking at the code around matchers the Underscore.js approach feels like the right approach. But the equality matcher is entangled in some other problems so we've not made progress yet.

Davis W. Frank
Owner

Take a look at the matchers_redo branch. We've moved to using Underscore's equality and it's improved our cases like this.

Kevin Locke

Thanks for working on it! Unfortunately, the problem appears to be unchanged. Using lib/jasmine-core/jasmine.js from matchers_redo (commit a34de5c) the error is thrown at jasmine.js:738. If I run grunt buildDistribution to create a new lib/jasmine-core/jasmine.js the error is thrown at jasmine.js:1252. In either case, it is when __Jasmine_been_here_before__ is assigned to the object under comparison.

Davis W. Frank
Owner

Very strange. Can you write a spec that shows the failure, using j$.matchersUtil.equal(...) ? I want to get that into matchersUtilSpec.

Kevin Locke

The way that I am provoking the failure is by comparing against a browser native object, so the test would have to be run in an XULRunner application. I could possibly simulate it by comparing an object with a setter property named __Jasmine_been_here_before__ which throws when a value is assigned, although that may be a bit contrived for what you are looking for. What do you think?

Davis W. Frank
Owner

The problem is not the comparison - it's in the pretty-printing of the error. We've changed how equality is checked, but the output of the error (serializing objects) is further down the backlog.

Look at matchersUtilSpec.js and see if you can get a test like that to pass or fail as you expect.

I think this issue has transmogrified into a new one.

Kevin Locke

I can certainly file another issue, if you think that's warranted. From my point of view, I just want expect(x).toEqual(y) to work when x and/or y is frozen and/or a WrappedNative object. Whether it's the equality comparison or the printing that's causing the error isn't really my area.

Unfortunately, I can't get the spec runner to work (by running rake jasmine - 292 of 296 specs fail, most with "j$ is not defined"). I'm probably doing something wrong, but I don't have the time to investigate right now. Here's an example of code that will fail:

describe("Immutable objects", function() {
  it("should be equal", function() {
    var obj1, obj2;

    obj1 = {};
    Object.defineProperty(obj1, "__Jasmine_been_here_before__", {
      set: function() { throw new Error("Can't modify this object"); }
    });
    obj2 = {};
    Object.defineProperty(obj2, "__Jasmine_been_here_before__", {
      set: function() { throw new Error("Can't modify this object"); }
    });

    expect(obj1).toEqual(obj2);
  });
});

Based on that, here's my guess at a spec:

it("compares objects which are immutable", function() {
  var actual = {},
    expected = {};

  // Throw on modify to emulate WrappedNative objects in XULRunner
  // Note:  frozen objects can also throw on modify in ES5 strict mode
  Object.defineProperty(actual, "__Jasmine_been_here_before__", {
    set: function() { throw new Error("Can't modify this object"); }
  });
  Object.defineProperty(expected, "__Jasmine_been_here_before__", {
    set: function() { throw new Error("Can't modify this object"); }
  });

  Object.freeze(actual);
  Object.freeze(expected);

  expect(j$.matchersUtil.equals(actual, expected)).toBe(true);
});
Davis W. Frank
Owner

Fantastic. That last spec is what I was misunderstanding.

But I do think that there are two problems - one that frozen objects fail the equality test and an other that says our PrettyPrinter fails for frozen objects - but since the reporter uses the Pretty Printer, you see a failure on toEqual().

The former should be fixed on master - I'll verify by adding a spec today. The latter is a bigger effort and we're pondering how to move away from the Pretty Printer right now.

Thanks for the patience and the spec.

Davis W. Frank
Owner

I've added this story to track a new Pretty Printer.

Kevin Locke

Great! Thanks for working on it! No hurry on fixing the Pretty Printer, I can work around it pretty well.

Davis W. Frank
Owner

Here's the commit: 8303c79

And it's passing. Notice I didn't add the "Jasmine has been here before" as I wanted to keep the PrettyPrinter out of this first fix.

Can we close this issue?

Kevin Locke

Sure. Do you want me to create another issue for the Pretty Printer, or do you just want to track it in Pivotal Tracker?

Davis W. Frank
Owner

It's in Tracker. :) Thanks for helping explain the issues.

Davis W. Frank infews closed this
Greg Cobb Gerg referenced this issue from a commit
Greg Cobb and Luan Santos Refactor prettyPrinter to work with immutable objects
[#50766813][#266]
c9e37a2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.