Add tests for Edit.focusHint

[pcardune]

Some of this is covered by focus-test, but since focusing is so complex, it would be nice to have some tests that just deal with how performEdit provides hints for which node to focus on after the edit has occurred. These can go in performEdit-test.ts

[schanzer]

@pcardune working on this now, and plan of attack is to add an expect() statement to each of the existing tests which checks to ensure that editor.getFocusedNode() is what it should be. From there, I'll add any additional edits to catch actions that don't yet have tests.

The problem is that the editor is just a CodeMirrorFacade, and therefore doesn't have that method available. Are you ok with me changing the editor type, or did you have a different approach in mind?

[pcardune]

I would say to avoid changing any interface for the sole purpose of writing a test.

I was imagining these being more like unit tests. Using editor.getFocusedNode() increases the amount of code under test, so there is more code to look at to figure out what broke if/when the tests fail. editor.getFocusedNode() just returns the what's in the redux store, but the redux store is only updated by dispatching performEdit(), which in turn calls commitChanges(), which is itself quite complicated. So if you use editor.getFocusedNode() in the test and the tests fails, then it's not immediately clear whether the problem is in commitChanges() or computeFocusNodeFromHint() or patch(), etc.

I was imagining just directly calling focusHint() on the Edit object and checking the return value directly. For example, you might add the following to the existing test for DeleteChildEdit:

  it("DeleteChildEdit removes a node from its parent", () => {
    const edit = edit_delete(ast, [...ast.rootNodes[1].children()][1]);
    expect(edit.toString()).toEqual("DeleteChild 2:13-2:26");
    const result = apply([edit]);
    if (!result.successful) {
      fail();
    }
    expect(edit.focusHint(result.value.newAST)).toEqual(...);
    ...
  });

Separately, it would be useful to have tests for computeFocusNodeFromHint() and computeFocusNodeFromChanges().

In the end, the line that you draw in the sand that determines what code is being tested is a highly debatable topic. You could argue (as many people do) that you should only write integration tests since that's how users will interact with your code and it's easier to completely rewrite/refactor the implementation without having to change any tests (yay!). At the same time, you could argue (as many people do) that you should just write unit tests that cover the smallest area of code possible (like an individual function), since it makes it easier to determine where the problem is when a test fails, generally encourages you to design better APIs that are easier to test, and can serve as documentation for how the code works (yay!). Finally, there is the giant gray area of what you should consider a "unit". It could be an individual function, or an individual file, or an individual collection of files that support a specific feature, or an individual npm package, etc.

I could be convinced that the entire collection of files in the edit directory should be the "unit" under test, in which case all tests would just call performEdit() and all the other code would be considered an implementation detail of performEdit(). But that's really a lot of quite complicated and quite critical code, so you would really need to take extra extra care to make sure the tests really covered all the possible behaviors / branches / errors. And not just covered in the sense that a particular line or branch was executed, but covered in the sense that it's effect was explicitly tested. It just seems easier to me to break it up into smaller pieces that are individually tested.

[schanzer]

@pcardune spent a few hours trying to track down the discontinuity between the way focusHint is constructed in the test vs. how it runs in the editor. I originally thought the culprit was a difference in the way edits are passed, but before the focusHint is computed these edits are coalesced in a way that results in the same hint being used no matter what. So why is the hint different when the same operation is performed in the test vs. the editor? I was able to trace it back to the creation of the DeleteChildEdit object:

To get some debugging output, I changed the focusHint method of DeleteChildEdit inside performEdits.js to be this:

  focusHint(newAST: AST) {
    console.log(this.prevId, newAST.getNodeById(this.prevId));
    return (this.prevId && newAST.getNodeById(this.prevId)) || "fallback";
  }

When making the same change in the editor that's made in the test, I get two different console outputs! The test output includes the prevId (3), but undefined for the node. In the live editor, I get a different prevId (f) and the node is retrieved successfully. As you can see from the method, this is the difference between the focusHint being a node or fallback.

I'm surprised to see that the id is different, and stumped as to why the node isn't getting returned. I'm guessing there is something about the way the tests are run that results in different state than the live version, but I don't know what that is. I've pushed the code with the debugging out to schanzer/fix-468.

[schanzer]

@pcardune I again modified the focusHint method above, to get some better output:

  focusHint(newAST: AST) {
    console.log('looking for ', this.prevId, 'in', newAST);
    if(this.prevId) console.log('found', newAST.getNodeById(this.prevId));
    return (this.prevId && newAST.getNodeById(this.prevId)) || "fallback";
  }

BEFORE THE EDIT:
When I open the live editor, switch to block mode and print out the AST, I see nodeIds starting at c. But printing out the starting ast in the test shows nodeIds starting at 0. Concern #1: why do the starting ASTs have different nodeIds? It looks like the full-blown editor is constructing and tearing down a lot more nodes during setup than CodeMirrorFacade.

AFTER THE EDIT:
The live editor and test both produce a new AST, and both trees' nodeIds start with c. The test AST's ids go straight from c to 13, but the editor's AST jumps from f to 14. The edits made in both contexts get coalesced into a single edit action with identical to/from fields, and only a single unify call is made. Concern #2: where is this discrepancy coming from?

The live editor goes looking for nodeId f, and the test looks for 3. In the live editor the node is found, but in the test it's not. This causes "fallback" to be returned, which causes the test to fail.

[pcardune]

This is great sleuthing. I'm not really surprised that the node ids are different in the tests. It's really not the goal of the tests to replicate the behavior of the live editor (they are unit tests, not integration tests), and the fact that the ids are different doesn't indicate that there is necessarily a bug.

I think the discrepancies you are running into, and the confusion you are having in the face of those discrepancies, is really an indication of the (over?) complexity of the system and how poorly defined the contracts are between different areas of code.

For example, the focusHint(newAST: AST) method takes an AST object, but the API doesn't actually enforce any rules about where that AST object came from, so you could start with some AST, apply the edits, and then pass the original AST to focusHint() and get something back. Or you could apply a bunch of different edits to an AST in sequence, and then call focusHint() on the very first edit with the AST that was produced after the very last edit. Both of these calls will execute just fine and return some value. That value might be "garbage" but nothing in the code is going to tell you that... the code simply wasn't written with these scenarios in mind. Worse, the "garbage" value might actually happen to be the same as the value you get when calling focusHint() correctly. Then the tests will pass and the application will work "by accident."

In an ideal world, the API is designed in such a way that it can't be used "incorrectly". In practice, that's rarely possible, and it's just a question of how close you can get with the tools available to you. Static types help, runtime error checking helps, certain design patterns help, documentation helps, but you'll always have some degree of "if you call this function in the wrong way, the expected behavior is undefined."

I think a good goal for you with these tests would be to:

1. Understand how the code in performEdits.ts was intended to be used
2. Document the intended usage
3. Write tests that exercise the intended usage

And to be a bit more specific about all of the above points, the goal would be to understand, document, and test the usage of performEdits.ts without referencing anything outside of performEdits.ts. If your answer to the question "how is it supposed to be used?" is "however it's currently being used", then that is the wrong answer. If I'm brand new to the code, I don't want to have to understand how all the code works in order to understand how part of the code works.

In going through the above steps, you might notice that performEdits.ts actually does a lot of stuff, and some of those things seem very unrelated to the other things it does, and it's really hard to explain it all in a straightforward way without complex diagrams. At that point it will be easier to see how you might (or might not) be able to refactor it into simpler components that are harder to use incorrectly. But I wouldn't pull too hard on that thread before getting the current implementation documented and tested.

FWIW, I think the difference in node ids is because the full editor generates several new copies of the AST when switching to block mode: 1) parse the text to generate an AST, 2) pretty-print that AST back to text and then parse the pretty-printed version back again to make sure you can do a full round trip. The tests operate at a layer of abstraction below the notion of "block mode" vs "text mode" so none of that needs to happen.

[schanzer]

@pcardune agreed that my first concern is just a function of the full editor doing the double-parse. I wasn't sure if the CMFacade skipped that, so I'm glad to hear this is expected. That means the issue could be second concern: the unify function is behaving differently, despite having an identical edit action. But even this isn't necessarily wrong, since subtle differences can change the hash and unify uses the hash as an index.

I think you're absolutely right that the most suspicious thing here is some kind of misalignment between which AST is used when. I'll start digging there first, and if I come up empty I'll look more closely at the differences in trees created by unify