Adding Iterator to nested objects data type break Snapshots

[jcubic]

Discussed in https://github.com/avajs/ava/discussions/2810

^{Originally posted by **jcubic** August 17, 2021} I have a project that uses linked lists (class Pair that have car and cdr as part of my lisp interpreter). Those pairs create nested objects structures and I'm comparing that object structure with a snapshot. Recently I've added this snippet of code that add iterator to my data type: ```diff + Pair.prototype[Symbol.iterator] = function() { + var node = this; + return { + next: function() { + var cur = node; + node = cur.cdr; + if (node === nil) { + return { value: undefined, done: true }; + } else { + return { value: cur.car, done: false }; + } + } + }; + }; ``` There is a bug in this code it should be `cur === nil` but after fixing it, it also shows an error in tests when comparing Pair structure with a snapshot. My question is how ava works with iterators vs objects with structure. Because the iterator is used only for lists and with Pairs, you can create any Trees. It should not break tests, but Ava thinks that the data has changed. Ava should not use iterator anywhere, it should compare my nested object structure instead. Here is a link to the broken test: https://travis-ci.com/github/jcubic/lips/builds/235460776 Here are the tests that are failing when reading snapshot: https://github.com/jcubic/lips/blob/devel/tests/macroexpand.scm The code is in Scheme (lisp dialect), but it compares JavaScript objects. I was trying to reproduce the issue by calculating the hash used by ava. But it works as expected, the hash is the same before and after adding Iterator: ```javascript var cbor = require('cbor'); var crypto = require('crypto'); var zlib = require('zlib'); function Pair(car, cdr) { this.car = car; this.cdr = cdr; } function Nil() {} var nil = new Nil(); var x = new Pair( new Pair( new Pair( 10, new Pair( 20, nil ) ), nil ), nil ); function hash(data) { const encoded = cbor.encodeOne(data, { omitUndefinedProperties: true, canonical: true }); const compressed = zlib.gzipSync(encoded); compressed[9] = 0x03; // Override the GZip header containing the OS to always be Linux const sha256sum = crypto.createHash('sha256').update(compressed).digest(); return sha256sum; } var before = hash(x); var ensure = hash(x); Pair.prototype[Symbol.iterator] = function() { var node = this; return { next: function() { var cur = node; node = cur.cdr; if (cur === nil) { return { value: undefined, done: true }; } else { return { value: cur.car, done: false }; } } }; }; var after = hash(x); console.log({before, ensure, match: Buffer.compare(before, ensure) === 0}); console.log({after, before, match: Buffer.compare(after, before) === 0}) ``` Also when I've tried to update the snapshots to see the difference, I've got this error: ``` Could not update snapshots for the following test files: ⚠ tests/test.js ``` This is my only JavaScript test file.

[novemberborn]

Snapshots are serialized and compared using https://github.com/concordancejs/concordance/. This does perform iteration.

t.deepEqual() uses the same algorithm (though with some practical differences due to serialization constraints). Does it show the same problem when you compare two lists? (Don't compare the exact same object since it'll short-circuit.)

cbor and such merely encode the serialization so that shouldn't have an impact.

[jcubic]

Will check my simple example with concordance and see if the iterator makes the difference.

[jcubic]

Something happens that I don't understand. I wanted to check the tests that were skipped. I've run them and everything is fine. The snapshot was not modified (at least test.js.md file).

[jcubic]

@novemberborn I've just found that the test passes but ava updated the snapshots files even that I didn't tell it to do so. Can you explain what is happening why it modify the snapshot files, it was breaking and not the tests pass but only because it updated the snapshots.

[novemberborn]

@jcubic that will be hard retroactively, unless you have both states recorded in different commits.

If AVA is happy now, without it updating snapshots, then it should be fine, right?

[jcubic]

I can commit a new snapshot on a branch. I was mainly asking if there is a case when AVA is updating the snapshot and ignore the options. It should throw error that snapshots don't match and not update the snapshot. Unless there is some mechanism for fixing the snapshot if it's broken. Because it should never update the snapshot unless there is a special condition in the code.

[novemberborn]

I agree it shouldn't update a failing snapshot assertion. There are other conditions though under which it adds new snapshots. What happened here is hard to determine without having a before and after.

[jcubic]

It will take more time because it seems that the snapshot got removed when I disable the tests with test.skip, because there is no diff, all new data is added. Will check in git History to get the old snapshot and see the difference.

[jcubic]

This may be some kind of broken state in Ava but I would like to have confirmation from you. Here is a branch that has broken snapshots https://github.com/jcubic/lips/tree/ava-bug The problem is that I'm not able to update the snapshots with:

nyc ava --update-snapshots --timeout 10m --verbose tests/test.js

I only see errors and nothing happens the snapshot files are not updated. I've tried to copy the snapshots directory run update and compare but I'm not able to update.

Can you check what is the status of the snapshots?

[jcubic]

I can also try to create a simplified example using code in the original post.

[jcubic]

I'm reopening. I think I've forgotten to do this. This is still an issue that needs to be investigated.

[novemberborn]

Aha! AVA 3 doesn't let you save snapshots when skipping tests. AVA 4 is better with this, so you could give that a try (npm i -D ava@next).

[jcubic]

Thanks for the update, will check with the current code. I've had few skipped tests I've added them when I've found that Ava has this feature, it looks better in logs if you see that there are disabled tests (they are not yet working, added before implementation), previously I was using a dummy function, I think it was test_ that does nothing.

I Will let you know if removing skipped tests allows running code from a branch that I have broken code. Will also provide if possible two examples of snapshots, before and after adding interator.

[novemberborn]

it looks better in logs if you see that there are disabled tests (they are not yet working, added before implementation)

You can use test.failing() for those. The suite will pass as long as the tests are failing, but when they start to pass, you need to remove .failing().

[jcubic]

I've updated the snapshot in my project https://github.com/jcubic/lips/commit/5b82bd1b6045746db552dca3ac40cc7e460e5343

I can also try to create basic reproduction using my original code in the issue if don't know what is the difference.

I don't understand the diff at all. Maybe after adding iterator somehow, the snapshots got reordered, I'm not sure if they are the same snapshots but updated.

Let me know if you need simpler reproduction, I can try to reproduce in simpler case.

[jcubic]

Also, I think that the data appear twice in the snapshot. in the FIrst snapshot that is a representation of:

(let ((++ (lambda (a b) (* a b)))) (++ 1 2))

lambda appears twice in the diff.

[jcubic]

I've tried for an hour to run concordance to see what is the output. But I'm not able to run the thing. All I have is:

./node_modules/ava/lib/chalk.js:7
        throw new Error('Chalk has not yet been configured')

I've tried to run this code:

import {set as setChalk} from './node_modules/ava/lib/chalk.js';

setChalk({level: 1});

import concordance from 'concordance';

import lips from './src/lips.js';

import {snapshotManager as concordanceOptions} from './node_modules/ava/lib/concordance-options.js';

lips.parse('(foo (bar baz) quux (1 2))').then(([result]) => {
    const data = concordance.describe(result, concordanceOptions);
    console.log(data);
});

I have no idea how to use the concordance library.

[novemberborn]

I'd appreciate a smaller reproduction.

The concordance-options module needs to resolve the color settings to decide how to do formatting. IIRC it's optional, or pass {}, or perhaps {maxDepth: Infinity}.

It may be more useful to log the formatted data: console.log(concordance.format(data)).

(I haven't used the low-level API in a long while so I may be wrong with the specifics.)

[jcubic]

I was able to reproduce the issue in a simple example, I will create an issue in the concordance project.

[novemberborn]

Per https://github.com/concordancejs/concordance/issues/76 I think this is by design.