qunitjs / qunit

🔮 An easy-to-use JavaScript unit testing framework.
https://qunitjs.com
MIT License
4.01k stars 780 forks source link

propEqual should treat TypedArray as arrays #1114

Open leobalter opened 7 years ago

leobalter commented 7 years ago

Tell us about your runtime:

What are you trying to do?

I'm trying to use deepEqual to compare an array with a typedarray instance

Code that reproduces the problem:

var { test } = QUnit;

test('float64Array', t => {
  var sample = new Float64Array([1, 2, 3, 4, 5, 6, 7, 8])
  var src = new Float32Array(sample.buffer, 0, 2);

  src[0] = 42;

  sample.set(src, 1);

  t.deepEqual(sample, [1.0000002464512363, 42, 1.875, 4, 5, 6, 7, 8])
})

If you have any relevant configuration information, please include that here:

What did you expect to happen?

being able to compare values between instances

What actually happened?

captura de tela 2017-03-13 17 40 11
trentmwillis commented 7 years ago

This seems reasonable to me, though I don't think there is an easy way to check if an object is a TypedArray (as in, I don't think you can do obj instanceof TypedArray). That said, we'd only need to perform this check if we're are comparing an Array with an Object.

zackthehuman commented 7 years ago

Unfortunately I don't believe there is a convenient way to check TypedArray-ness unless you check instanceof for all of the various subclasses. Supporting this would be quite helpful for some of the tests I want to write for my emulator project.

Is this something that would be good for a new contributor?

trentmwillis commented 7 years ago

@zackthehuman, I believe you are correct. I don't think this should've very difficult to implement,so if you'd like to take a stab at it, go for it!

I think the primary changes will be in QUnit.equiv and adding additional tests.

leobalter commented 7 years ago

we should not rely on instanceof. If we want to limit checks for similar subclasses, we could use better approaches as getting the prototype chain.

In my case, what I would like is to have equiv to iterate through keys and values, regardless it's a TypedArray or Array or any other iterable object.

The problem: would this be a breaking change for equiv and deepEqual? Should we have a new API method for that?

In my suggestion, equiv would be valid to these cases:

var a = [ 42, 39, 262 ];
var b = new Int32Array([42, 39, 262]);
var c = { length: 3, "0": 42, "1": 39, "2": 262 };
var d = (function *() { yield 42; yield 39; yield 262; })();

Btw, cases a, b, and d, are good for similar for of loops:

for (let x of a) console.log(x);
for (let x of b) console.log(x);
for (let x of d) console.log(x);

But c is valid to create a new TypedArray, e.g.: new Int32Array(c), and Arrays: Array.from(c).

They share some equality I could use in my assertions.

leobalter commented 7 years ago

I'm probably bikeshedding here, but I might want my request in two changes:

Extend deepEqual/equiv to compare Arrays with TypedArrays

Iterate over array-like indexed properties when comparing TypedArrays with Arrays. We can detect TypedArrays by the shared %TypedArray% prototype/constructor:

Int32Array === Int8Array; // false
Object.getPrototypeOf(Int32Array) // TypedArray
Object.getPrototypeOf(Int32Array) === Object.getPrototypeOf(Int8Array); // true

// check
let TypedArray = Object.getPrototypeOf(Int8Array).constructor;
let res = myObj instanceof TypedArray;
if ( res ) {
  // ... Iterate over myObj and the given Array or other TypedArray number indexes.
  // Array.from (or rest [...myObj]) shortens the way to compare the number indexed properties.
  // After that: compare __non-number enumerable properties__.
}

Create a new assertion to compare iterators

We should name it with similar or something close, I'll explain why as well.

assert.similarIteration = function(a, b) {
  var res = QUnit.equiv([...a], [...b]);
  // use `res` to parse result
};

Why similar? Because of other iterators. Imagine strings, or results from generators:

var d = (function *() { yield 42; yield 39; yield 262; })();
var gen = function *(x, y, z) { yield x; yield y; yield z; }; // this is not called yet

assert.similarIteration(d, gen(42, 39, 262));
// gen might return different iterators, and iterators might behave totally different
// not predictable as Arrays where length is already defined.

// The same happens with Objects with [Symbol.iterator] properties.
zackthehuman commented 7 years ago

Are there any concerns about adding TypedArray support for browsers that don't support TypedArrays (basically IE before IE 10)?

leobalter commented 7 years ago

QUnit's concern is only restricted to know if TypedArrays are supported by the current environment. If it's not, skip these checks.

trentmwillis commented 7 years ago

FWIW, here's an older issue with some thoughts on this: https://github.com/qunitjs/qunit/issues/672

Krinkle commented 4 years ago

I think I'd decline this for deepEqual, which uses strict comparison, but the class-agnostic use case falls very well under propEqual. I think it would make sense for propEqual to treat array-like objects equally and thus normalise all of these to array, the same for other iterators and generators probably.