Render array moves with arrows #333

papandreou · 2016-09-11T22:22:40Z

Affects the array-like diff and the <array-like> to [exhaustively] satisfy <array-like> assertion.

Here's what it'll look like in unexpected-sinon: https://github.com/unexpectedjs/unexpected-sinon/compare/feature/arrayMoves

And unexpected-dom: papandreou/unexpected-dom@11b7199 (only one of the existing tests seemed to contain a move)

papandreou · 2016-09-11T22:32:15Z

documentation/api/addAssertion.md

+  ┌─>
+  │   3,
+  └── 2, // should be moved
+      4 // should be undefined


Oops, it shouldn't say "should be undefined" here.

sunesimonsen · 2016-09-12T05:20:16Z

This looks pretty insane 😀 I'm still concerned about performance, complexity and verbosity. But we can probably put in stop gaps so this will only run in cases where it is manageable.

Would it be possible to use unicode arrows to avoid using 3 lines to represent one connection.

sunesimonsen · 2016-09-12T05:20:55Z

http://jrgraphix.net/r/Unicode/2190-21FF

papandreou · 2016-09-12T08:07:09Z

Hmm, those unicode arrows don't seem to compose very well:

↱
∣
↳
═⇒
⇑
‖
⇓
⋯⇢

sunesimonsen · 2016-09-12T08:23:03Z

You are right, you can draw perfect boxes but the arrows does not match 😢

papandreou · 2016-09-12T08:39:28Z

Fixed those "should be undefined" bugs.

I'm still concerned about performance, complexity and verbosity.

Yeah, me too :). I tried to address the performance and complexity concerns by consolidating the arrow handling in a single style that's used by both <array-like> to satisfy <array-like> and the array-like diff, and by not using the merge style when the are no arrows or just one arrow to render.

As for the verbosity, it seems like the existing heuristic for falling back to an index-by-index comparison is still doing a good job here. We want to additionally limit the number of moves to render using arrows. In my opinion, none of our existing tests seem to exhibit any bad behavior, though.

alexjeffburke · 2016-09-12T09:29:28Z

I agree to some extent with @sunesimonsen about verbosity etc, but I am kind of compelled at the sight of this output with the -sinon plugin. Visually it's probably most useful when things are naturally strongly ordered, and when talking about function calls we definitely think of a series of them as having an order. That might explain why I'm less sure about the -dom output with this.

Anyway, I'd be tempted to have a way of turning this on/off per diff and it might be worth considering the default.

papandreou · 2016-09-12T09:34:03Z

Visually it's probably most useful when things are naturally strongly ordered

Not sure I understand? When comparing arrays, the order is always significant per the semantics?

Anyway, I'd be tempted to have a way of turning this on/off per diff and it might be worth considering the default.

You mean in the code (when subclassing the array-like type) or as a user-facing option?

alexjeffburke · 2016-09-12T09:41:00Z

@papandreou Ah sorry I didn't immediately twig that DOM example was a NodeList. I think I was considering the case where something was declared as an array but the order might matter less, though having said that I can't think of a good example that exists now. Perhaps if we did add support for "these mocks but in any order" to -mitm or equivalent we'd always want a diff without arrows?

So yes, I probably meant a within the code opt in/out as appropriate.

papandreou · 2016-09-12T09:45:21Z

Perhaps if we did add support for "these mocks but in any order" to -mitm or equivalent we'd always want a diff without arrows?

Agreed. In that case the matching semantics are also different, though -- that's why I made http://unexpected.js.org/unexpected-set

By the way, I think the arrows will be really nice if we make that change to unexpected-mitm that we discussed where we would initially pick the first request that matches the spec, but if the requests came in the wrong order, we would error out after the test has completed.

sunesimonsen · 2016-09-12T18:16:49Z

I have a feeling that it would be wise to back out of using this for the following cases:

too many crossing arrows
arrows becomes too long
more than 3 levels

papandreou · 2016-09-12T18:37:29Z

I know this is in the nitpick department, but I would prefer:
expected [ 1, 3, 2, 4 ] to be sorted

[
   1,
┌─>
│  3,
└─ 2, // should be moved
   4
]

Sure thing. I left those spaces there to honor the indent setting of the type, but I agree that it looks better without it when the arrows are there.

sunesimonsen · 2016-09-12T18:40:41Z

@papandreou sorry I deleted my comment as I realised that cross arrows would be problematic with my suggestion. But We can always tweak that.

sunesimonsen · 2016-09-12T18:42:13Z

@papandreou you can still honour the indent, you can just use it as available space.

sunesimonsen · 2016-09-12T18:43:05Z

But sorry, we should not bikesheet about looks right now :-)

papandreou · 2016-09-12T18:45:30Z

I realised that cross arrows would be problematic with my suggestion. But We can always tweak that.

The current implementation is allocating as much horizontal space as it needs (except that it doesn't try to do any "packing", so getting rid of the indentation doesn't cause any trouble in itself. I removed it in 6a97050

papandreou · 2016-09-12T18:54:21Z

I have a feeling that it would be wise to back out of using this for the following cases:

too many crossing arrows

arrows becomes too long

more than 3 levels

Agreed, seems like no upper limit is currently enforced:

sunesimonsen · 2016-09-12T18:59:26Z

hehe 🎉

papandreou · 2016-09-14T12:38:04Z

Rebased on top of the array-changes and array-changes-async fixes (both this branch and the branches in those repos).

papandreou · 2016-09-14T12:38:23Z

Andreas Lind @papandreou 14:21
And we still need to work out how many arrows are "too many" so we can bail out of rendering them.

Sune Simonsen @sunesimonsen 14:21
@papandreou I think it is more about the number of layers they contribute
3 or 4 would be a good measure in my opinion.

papandreou · 2016-09-15T12:01:37Z

I think this is in a good state now. It requires unexpectedjs/array-changes#5 and bruderstein/array-changes-async#5 to be merged and new major versions of array-changes and array-changes-async to be released.

sunesimonsen · 2016-09-15T19:58:46Z

@papandreou that is good news. I'll take a look at the code soon.

papandreou · 2016-09-15T20:00:17Z

Thanks -- I'm curious if you have a better idea of how to implement the merge style, and whether we want something like it in magicpen.

sunesimonsen · 2016-09-15T20:24:10Z

@papandreou merge is overlaying the output of one pen on top of another - right?

If yes that should probably be in magicpen as it is very generic and useful.

At some point I was thinking about these kind of things - centering the output of a pen on top of another and other layout stuff. But with a method like this in magicpen, you could probably make the rest as a plugin.

papandreou · 2016-09-15T20:31:58Z

merge is overlaying the output of one pen on top of another - right?

Yeah, that seemed much easier than rendering all the arrows in one go.

sunesimonsen · 2016-09-15T20:32:41Z

Yes makes a lot of sense.

papandreou · 2016-09-16T08:15:31Z

@bruderstein Are you OK with releasing new versions of array-changes(-async) that would potentially break unexpected-htmllike? Should we explore options to move it (and/or the adaptors) to utilize the modules via the array-like type, or would you rather reimplement the arrow rendering?

papandreou · 2016-09-17T09:03:04Z

Possible future improvements that have come up:

@sunesimonsen noticed that the move arrows always point upwards. This seems to be an artifact of arraydiff's algorithm, and it sometimes leads to more arrows than are really necessary. Fixing this would probably be quite a bit of work.
Swapped items could be rendered as one arrow -- probably blocked on the same thing.
Multiple moved items (eg. {type: 'move', howMany: 3, ...} from arraydiff) could possible be rendered with one arrow-ish thing, but is currently turned into one per item.

sunesimonsen · 2016-09-17T09:10:55Z

Very interesting idea of creating output that follows the array diff more closely.

papandreou · 2016-09-17T22:01:21Z

It's certainly doable. I'm just not sure that it'd be obvious that

expected [ 3, 1, 2 ] to equal [ 1, 2, 3 ]

┌─▷
│   3,
├── 1, // should be moved
└── 2 // should be moved

is equivalent to

┌───▷
│ ┌─▷
│ │   3,
└─├── 1, // should be moved
  └── 2 // should be moved

rather than:

┌───▷
│ ┌─▷
│ │   3,
│ └── 1, // should be moved
└──── 2 // should be moved

sunesimonsen · 2016-09-18T05:43:21Z

I think you are right. The single arrows are easier to understand.

sunesimonsen · 2016-09-19T20:01:26Z

lib/assertions.js

@@ -1044,59 +1044,68 @@ module.exports = function (expect) {
                            if (subjectType.indent) {
                                output.indentLines();
                            }
-                            changes.forEach(function (diffItem, index) {
+                            var packing = utils.packArrows(changes); // NOTE: Will have side effects in changes if the packing results in too many arrow lanes
+                            output.arrowsAlongsideChangeOutputs(packing, changes.map(function (diffItem, index) {


It really has nothing to do with the PR, but I think we have crossed the line where we should split up the assertions in separates files (Not in this PR - but after). After we split the assertions into separate files, I think something like this would be much more readable if it was split into some more helper methods.

sunesimonsen · 2016-09-19T20:04:43Z

lib/assertions.js

-                                                    this.append(valueDiff.amend(delimiterOutput));
+                                var type = diffItem.type;
+                                if (type === 'moveTarget') {
+                                    return output.clone();


It feels like there should be a magicpen construct to map over an array of items and produce output for each item. I know you are just cloning the output, but it feels a bit unsafe that you might append to the outside output by accident. This is just thoughts on how we might refactor later.

sunesimonsen · 2016-09-19T20:06:22Z

lib/styles.js

+        for (i = 1 ; i < options.width ; i += 1) {
+            this.text(i === options.width - 1 && options.direction === 'down' ? '▷' : '─', styles);
+        }
+    });


Very nice 👍

We should probably make a magicpen-arrow plugin.

If we can make it a bit more generic, and if unexpected-htmllike/-react cannot reach this feature through the array-like type, I agree.

But it might also mean implementing a lot of features that we wont use.

sunesimonsen · 2016-09-19T20:07:12Z

lib/styles.js

+                }
+            } while (contentLeft);
+        }
+    });


As we talked about, this style should move to magicpen core when it has proven it's stability.

sunesimonsen · 2016-09-19T20:12:59Z

lib/styles.js

+                this.block(function () {
+                    this.merge(arrows);
+                });
+            }


If would be better if this was just:

this.block(function () { this.merge(arrows); });

Magicpen should be smart enough to avoid the extra block.

and merge should be smart enough to handle one pen.

The way merge is implemented, it takes an array of pens and produces a fresh pen with the merged output. It does handle the single pen case, but I'd like to avoid creating a copy here. My initial attempt at implementing the merging was to merge one or more other pens into the current pen, but that turned out to be harder to implement and not as understandable.

My initial attempt at implementing the merging was to merge one or more other pens into the current pen, but that turned out to be harder to implement and not as understandable.

Ohh I though it worked that way. That is probably how I want it to work in magicpen.

Hmm, I agree that would be the most useful API-wise.

sunesimonsen · 2016-09-19T20:15:52Z

lib/types.js

+                        }
+                    });
+                }
+            }));


This method feels a lot like Déjà vu, it would be nice of the two method shared more common parts. But let's look at if it is possible to refactored it afterwards.

Yeah, a long time ago we made the decision to keep the array/object diffs separate from the generation of the 'to satisfy' diffs. Since then there has been a lot of fluctuation back and forth, but I think they're still sufficiently different that it could pose a challenge to reconsolidate them.

If we decide to go for #330 they would align closer.

I'm not saying that we should reconcile them, just that we should look for common parts that we can extract :-)

sunesimonsen · 2016-09-19T20:17:44Z

package.json

-    "array-changes": "1.3.1",
-    "array-changes-async": "2.2.1",
+    "array-changes": "unexpectedjs/array-changes#e2385e5b3130e7c715dee6d77489c78565833bcb",
+    "array-changes-async": "bruderstein/array-changes-async#8c965f125be6f45604ef764a7750b4c23eafd9f9",


We of cause needs these released before this can be merged. But I guess we are waiting on an answer from @bruderstein - right?

Yes, I'd like him to sign off on it so I don't create any stumbling blocks for unexpected-htmllike and unexpected-react.

sunesimonsen · 2016-09-19T20:19:01Z

lib/styles.js

+        for (i = 1 ; i < options.width ; i += 1) {
+            this.text(i === options.width - 1 && options.direction === 'down' ? '▷' : '─', styles);
+        }
+    });


We should probably make a magicpen-arrow plugin.

sunesimonsen

This seems to be ready to merge after the dependencies has been released. But we should really take unexpected for a refactoring dance afterwards.

I'm still a bit curious about the performance effects, can we get any kind of measurements, comparing two arrays with 500 items. We should probably skip the diff for arrays with more then 50 items - I don't really know what the limit should be.

Are we completely sure the code does the right thing? Should we create a unexpected-check test that compares random arrays, when it fails checks that the output contains the same number of moves, removes and inserts as the array-changes?

sunesimonsen · 2016-09-19T20:20:55Z

test/styles/overlay.js

@@ -0,0 +1,24 @@
+/*global expect*/
+describe('overlay', function () {


This should be merge style.

papandreou · 2016-09-19T22:48:47Z

Thanks for reviewing!

I'm still a bit curious about the performance effects, can we get any kind of measurements, comparing two arrays with 500 items. We should probably skip the diff for arrays with more then 50 items - I don't really know what the limit should be.

I tried writing a quick script:

var a1 = new Array(500);
var a2 = new Array(500);
for (var i = 0 ; i < 500 ; i += 1) {
    a2[i] = i;
    if (i * 2 < 500) {
        a1[i * 2] = i;
    } else {
        a1[(i - 250) * 2 + 1] = i;
    }
}
var expect = require('./');

for (var i = 0 ; i < 10 ; i += 1) {
    try {
        expect(a1, 'to equal', a2);
    } catch (e) {}
}

expect(a1, 'to equal', a2);

To my surprise it actually runs faster with these changes than on master, 4.3 seconds vs. 5.1 seconds. The <array-like> to [exhaustively] satisfy <array-like> assertion already falls back to index-by-index if either the subject or value has more than 10 items, so I'm not really worried about that.

Are we completely sure the code does the right thing? Should we create a unexpected-check test that compares random arrays, when it fails checks that the output contains the same number of moves, removes and inserts as the array-changes?

I'm reasonably sure it does the right thing, but it's an interesting idea to exercise it like that.

sunesimonsen · 2016-09-20T07:26:01Z

Creating an unexpected-check test might be a bit more challenging than I initially imagined as you need to take the fallback to indexed diffing into account. But you could make a test that asserts that the either don't contain an arrow or that the diff is consistent with the array-changes. But I'm completely okay with merging this without such a test.

sunesimonsen · 2016-09-20T07:27:56Z

The interval packer already has randomized tests so that part should be good: https://github.com/One-com/greedy-interval-packer/blob/master/test/randomized.spec.js

…ce for the arrows.

…nserts and moveSources to be ordered differently (harmless).

…change outputs.

papandreou · 2016-09-26T06:55:37Z

Fixed up and rebased. Merging if CI is green.

papandreou reviewed Sep 11, 2016
View reviewed changes

papandreou force-pushed the feature/arrayMoveArrows branch from 9b2636d to e422777 Compare September 14, 2016 12:37

papandreou changed the title ~~WIP: Render array moves with arrows~~ Render array moves with arrows Sep 15, 2016

This was referenced Sep 15, 2016

Preserve moved items. unexpectedjs/array-changes#5

Merged

Preserve moved items. bruderstein/array-changes-async#5

Merged

sunesimonsen reviewed Sep 19, 2016

View reviewed changes

sunesimonsen approved these changes Sep 20, 2016

View reviewed changes

papandreou added 9 commits September 26, 2016 08:51

Update array-changes-async to 3.0.0 and array-changes to 2.0.0.

0738cf6

array-like: Render moves with arrows

4357884

Use greedy-interval-packer to avoid reserving too much horizontal spa…

eac213a

…ce for the arrows.

Render arrow heads with ▷ instead of >

38f0c46

Update expected output in a test where the array-changes fix causes i…

8966ca6

…nserts and moveSources to be ordered differently (harmless).

Pack using the change indices instead of the measured heights of the …

accb2f2

…change outputs.

Separate the packing from the arrowsAlongsideChangeOutputs style.

2110adf

Make sure that we render a max of 3 arrow lanes.

b9839d8

Avoid trailing whitespace when the first change is a moveTarget item.

917723b

papandreou force-pushed the feature/arrayMoveArrows branch from b8fb815 to 917723b Compare September 26, 2016 06:54

papandreou merged commit 75b0c16 into master Sep 26, 2016

papandreou deleted the feature/arrayMoveArrows branch September 26, 2016 07:18

		@@ -0,0 +1,24 @@
		/global expect/
		describe('overlay', function () {

Render array moves with arrows #333

Render array moves with arrows #333

Conversation

papandreou commented Sep 11, 2016 • edited Loading

Choose a reason for hiding this comment

sunesimonsen commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016

papandreou commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016

papandreou commented Sep 12, 2016

alexjeffburke commented Sep 12, 2016

papandreou commented Sep 12, 2016

alexjeffburke commented Sep 12, 2016

papandreou commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016

papandreou commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016 • edited Loading

sunesimonsen commented Sep 12, 2016

papandreou commented Sep 12, 2016 • edited Loading

papandreou commented Sep 12, 2016

sunesimonsen commented Sep 12, 2016

papandreou commented Sep 14, 2016

papandreou commented Sep 14, 2016

papandreou commented Sep 15, 2016

sunesimonsen commented Sep 15, 2016

papandreou commented Sep 15, 2016

sunesimonsen commented Sep 15, 2016

papandreou commented Sep 15, 2016

sunesimonsen commented Sep 15, 2016

papandreou commented Sep 16, 2016

papandreou commented Sep 17, 2016

sunesimonsen commented Sep 17, 2016

papandreou commented Sep 17, 2016 • edited Loading

sunesimonsen commented Sep 18, 2016

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

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

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

sunesimonsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papandreou commented Sep 19, 2016

sunesimonsen commented Sep 20, 2016

sunesimonsen commented Sep 20, 2016

papandreou commented Sep 26, 2016

papandreou commented Sep 11, 2016 •

edited

Loading

sunesimonsen commented Sep 12, 2016 •

edited

Loading

papandreou commented Sep 12, 2016 •

edited

Loading

papandreou commented Sep 17, 2016 •

edited

Loading