mcmire / super_diff Goto Github PK

At work, escaped color sequences are showing up on CircleCI when tests fail, even though RSpec has been configured to not show color.

Importing super_diff/rspec technically works in Cucumber but diffs aren't showing for some reason.

Currently we use a different algorithm to inspect two complex objects on a single line and then generate a diff of those objects. Here is a good example of this:

Differing objects.

Expected: #<SuperDiff::Test::Player:0x7fcdb0a77440 @handle="martymcfly" @character="mirage" @inventory=["flatline", "purple body shield"] @shields=0.6 @health=0.3 @ultimate=0.8>
  Actual: #<SuperDiff::Test::Player:0x7fcdb0a77300 @handle="docbrown" @character="lifeline" @inventory=["wingman", "mastiff"] @shields=0.6 @health=0.3 @ultimate=0.8>

Diff:

  #<SuperDiff::Test::Player {
-   handle: "martymcfly",
+   handle: "docbrown",
-   character: "mirage",
+   character: "lifeline",
    inventory: [
-     "flatline",
+     "wingman",
-     "purple body shield"
+     "mastiff"
    ],
    shields: 0.6,
    health: 0.3,
    ultimate: 0.8
  }>

In this case, "Expected" vs "Actual" uses deep inspection to generate an inspection of the two objects, whereas the diff uses an entirely different algorithm. Thus, you get SuperDiff::Test::Player:0x7fcdb0a77440 vs SuperDiff::Test::Player. Maybe there's a way we can make use of the pretty printing algorithm to generate a diff?

The CommandRunner is copied from my work with shoulda-matchers, and I imagine using it other places. Keeping it here bloats the codebase.

For instance, instead of producing this diff:

  {
    "data" => {
-     "getCurrentAccount" => {
-       "key" => "49b471a566d7762da44778ff63526401",
-       "firstName" => "Elliot",
-       "lastName" => "Winkler"
      }
    },
+   "errors" => [{ "type" => "unknown", "message" => "Something bad happened", "details" => nil }]
  }

It would produce this instead:

  {
    "data" => {
-     "getCurrentAccount" => {
-       "key" => "49b471a566d7762da44778ff63526401",
-       "firstName" => "Elliot",
-       "lastName" => "Winkler"
      }
    },
+   "errors" => [
+     {
+       "type" => "unknown",
+       "message" => "Something bad happened",
+       "details" => nil
+     }
+   ]
  }

Currently, respond_to is not colored like eq .

We already sort keys for ActiveRecord::Base instances, so it may make sense to sort hash keys too.

What I mean by collapsed output is this:

Error: ...

Expected: ...
Got: ...

Breakdown:
- *["foo"][1]["bar"][:baz]: Differing strings.
  - Expected: ...
  - Got: ...

instead of this:

Error: ...

Expected: ...
Got: ...

Breakdown:
- *["foo"]: ...
  - *[1]: ...
    - *["bar"]: ...
      - *[:baz]: Differing strings.
        - Expected: ...
        - Got: ...

In other words, it would collapse everything until it gets to a node beyond which we can no longer recurse (a simple type, in other words).

Currently, OperationSequence is the only class for which we do not have a factory function. In theory we shouldn't need a factory function for DiffFormatter, as operational sequences have a to_diff method and we should be able to use that. Can we extend this pattern to find the correct OperationSequence for an OperationalSequencer? And find the right OperationalSequencer for a Differ? That may obviate the need to repeat the applies_to? logic for each stratum of class in the hierarchy, as we have to do that right now (and it's a bit inconsistent).

Use "explicit mode" so we're not monkeypatching Object.

This doesn't appear great when setting the terminal background to a light colorscheme:

Right now we're getting output like:

+   [#<ShippingAddress
+   id:
+   1,
+   name:
+   "Rosa
+   Diaz",
+   phone:
+   "639-351-1301",
+   company:
+   nil,
+   line_1:
+   "111
+   Park
+   Place",
+   line_2:
+   "#13",
+   city:
+   "Denver",
+   state:
+   "CO",
+   zip:
+   "12345",
+   country:
+   "US"

The diff produced from RSpec's match matcher failing seems to include a duplicate of the actual data, especially if the actual data is a HashWithIndifferentAccess:

One example is the String differ that some dude wrote and posted to the RSpec issue tracker.

Another example is an OrderedHash differ that works very similar to the Hash differ.

In other words, the current differs need to be extracted into classes somehow.

For instance, if a class merely accepts a bunch of values or keyword values and sets instance variables, and we have two instances of the same class, it might be nice if the gem automatically compared them using instance variables instead of having to write a custom differ for that class.

https://github.com/mcmire/super_diff/blob/master/lib/super_diff/rspec/monkey_patches.rb#L591

This seems to happen if the given block doesn't actually raise an error.

This is gonna be a big one...

Rubocop is getting super annoying. I end up having to tweak the rules a lot and they're just not working for me. I really like the idea of using an autoformatter. The best candidate seems like rubyfmt. I'm not sure when it'll be available, but I can't wait.

If a test like the following fails:

expect(person.shipping_addresses.order(:id)).to match([
  existing_shipping_address
])

then the diff should skip over the fact that person.shipping_addresses.order(:id) returns a relation object and not an array.

This could look a bit better:

When printing:

Expected: ...
       to eq: ...

The "actual" value is in red and the "expected" value is in green. Shouldn't it be the other way around? (i.e. the actual value should be assumed to be the right one).

Note that this means we'd have to flip the colors in the diff as well, and update the legend.

There are a few different kinds of classes that have some duplication in them. Making a new strategy is a bit painful because you end up repeating yourself and only a small part of the class changes. Especially with DiffFormatters and using Collection.

Right now we have tests for comparing objects, but it uses the same class for both objects. We could have a case, however, where we're trying to compare totally different objects. We should make sure that the correct output is produced. (I don't think we should show a diff if you're comparing two objects, unless those objects are inside of another data structure.)

Using match + a_collection_containing_exactly doesn't seem to be producing a useful diff. Let's look into this.

There are no tests for "assuming color is disabled".

• When diffing
• When deep inspecting

No one needs another library to colorize text, but keeping it here bloats the codebase.

Unfortunately csi is already taken as a gem name. Something else?

When comparing the following

A: {:fiz => "gram", 1 => {2 => :sym}}
B: {42 => {:raz => "matazz"}, :fiz => "graeme", 1 => 3}

Output should be:

- *[42]: Expected to not be present, but found (value: {:raz => "matazz"})
- *[:fiz]...

Currently keys not found in A are listed at the very end.

This may be tricky since we're using Appraisal. Can use rubyfmt's workflow file as inspiration, but will have to do research on this first.

By turning off all of RSpec's colorization and then turning it on in select places, we've made it more difficult to know how to interpret unhandled errors.

Red and green connote good and bad. It means that the "actual" value is correct and the "expected" value is not. Well, who's to say? Sometimes it's the other way around. So maybe we shouldn't use overloaded colors in this way.

Currently if you have two arrays:

a = %w(a b c d)
b = %w(1 2 a b)

and you diff them, the differ will tell you this, basically:

0: "a" != "1"
1: "b" != "2"
2: "c" != "a"
3: "d" != "b"

Now, ideally, this is not very helpful. We should be able to figure out that "1" and "2" were inserted before "a" and that "c" and "d" were removed. So maybe the output would be exactly that:

- "1", "2" inserted before "a"
- "b", "c" deleted after "b"

Notice that we aren't listing the array indices anymore, since those are no longer meaningful.

This actually sounds like a proper use case for the LCS algorithm.

If there is a bug in the gem such that diffing, or even our custom overrides for matchers, don't work correctly, should we fall back to RSpec so as not to disrupt the developer?

Right now the JRuby tests take 9 times as long to run as the MRI tests. This is probably because for each integration test, we spawn a sub-ruby, which is a pretty big performance hit. In fact, JRuby recommends not doing this. There are ways to get around this, but perhaps the simplest way is to stuff all of the integration tests into a single file? This means that only one sub-ruby would be spawned. The results of each test would then be collected and compared to expected outputs. This would likely create some kind of Frankenstein in terms of the integration tests, so maybe this isn't worth it for now?

Currently, have_attributes is not colored like eq .