Simple expression parser tests fail when executing out-of-order

[jlindermeir]

When executing the "Typical setup" test case of the simple expression parser without running the "Most simple setup" case first, it fails with the following error:

 FAIL  tests/CodeCompletionCore.spec.ts (78.342 s)
  ● Code Completion Tests › Simple expression parser: › Typical setup

    expect(received).toEqual(expected) // deep equality

    - Expected  - 4
    + Received  + 1

    - Array [
    -   10,
    -   7,
    - ]
    + Array []

      513 |             expect(candidates.tokens.has(ExprLexer.LET)).toEqual(true);
      514 |
    > 515 |             expect(candidates.tokens.get(ExprLexer.VAR)).toEqual([ExprLexer.ID, ExprLexer.EQUAL]);
          |                                                          ^
      516 |             expect(candidates.tokens.get(ExprLexer.LET)).toEqual([ExprLexer.ID, ExprLexer.EQUAL]);
      517 |
      518 |             // 2) On the variable name ('c').

      at Object.<anonymous> (tests/CodeCompletionCore.spec.ts:515:58)

This can be reproduced by simply deleting the "Most simple setup" case and running npm run test.

As far as I can see, the problem stems from two issues:

• Is there a mistake in the "typical setup" case? If the ExprLexer.ID and ExprLexer.EQUAL tokens are in ignoredTokens of the code completion core, is it expected that they are included in the following lists of the candidate tokens?
• The tests are order dependent because the follow sets are cached in the followSetsByATN field, which is static and is therefore shared between different instances of the class. Maybe it makes sense to reset the field at the beginning of each test case to ensure a reproducible state? Also, the cache uses only the parser name as the key, which does not take configuration changes, e.g. different ignoredTokens into account.

In any case, thanks for the great work! I'm currently working on a python port of the library for my employer, and hope to contribute it once it is done. Please let me know if I can help with fixing this bug 🙂

[mike-lischke]

Yes, looks like the tests are not isolated, as they should be. But I think that's not tricky to solve. I would much appreciate if you could file a PR for that while you are at it.

And of course once you are done with your port I'll happily take it as another contribution!

[jlindermeir]

Hey Mike, sorry for the delay. I'll see what I can do.
To make the PR, could you answer the following questions?

• Is the test case in question correct as it is now, even when run in isolation? I.e. should the ID and EQUAL tokens be in the candidates, even if they are in the ignoredTokens set?
• How should we isolate the test cases? I can think of two options: We could either make the followSetsPerATN a per-instance field, or add a method to reset it and call that at the beginning of each test case. I'd prefer the former option, as that would also allow us to get rid of the fuzzy per-parser caching, which currently only uses the parser name and therefore does not take configuration changes into account.

We'll try to add the python port soon!

[mike-lischke]

Good questions. Ignored tokens should not appear in the candidate lists, otherwise you wouldn't need to add them to the ignore list. It's not clear how this could have been accepted in the past.

Having a per instance cache for the follow sets seems counterproductive. The ATN is static and lives during the entire application lifetime. Parsing different input doesn't invalidate it or change it. Only the DFA cache changes while parsing is going on, but that has no influence on the ATN walk the c3 engine is performing when looking for candidates.