Improve integration tests diagnostics on error

[ikappaki]

cc'ing @benjamin-asdf

Hi,

could you please review follow up patch to #3274 to improve on the integration tests diagnostics on error

Changelog

1. Also poll eval-err when expecting a response from the server.
2. Enable nREPL messages logging and dump all buffers contents on error.

Currently there is a break in the jack-in shadow-cljs test due to #3282 that is very terse saying that the test timedout waiting for a response from the server.

jack in to shadow

Traceback (most recent call last):
;; ...
error: (error ":cider-itu-poll-until-errored :timed-out-after-secs 10 :waiting-for eval-out")

this by itself does not provide much information on the error, we need more context. Thus the first change this patch introduces is to also poll for eval errors, which gives now back the following:

FAILED: Expected `eval-err' to be `equal' to `nil', but instead it was `("Syntax error compiling at (*cider-repl Temp\\lsvViC:localhost:59047(pending-cljs)*:1:15).
Unable to resolve symbol: *clojurescript-version* in this context
")' which does not match because: (different-types ("Syntax error compiling at (*cider-repl Temp\\lsvViC:localhost:59047(pending-cljs)*:1:15).
Unable to resolve symbol: *clojurescript-version* in this context
") nil).

This means the nREPL server could not resolve the *clojurescript-version* variable that it requested to evaluate earlier with (cider-interactive-eval "(print :cljs? (some? *clojurescript-version*))" ;; ....

The shadow repl starts it life as a clj REPL and then switches to cljs. In the test we first wati for the REPL to switch to cljs with (cider-itu-poll-until (cider-repls 'cljs nil) 120) (i.e. polling for up to 120secs until it can retrieve the cljs repl or erroring out otherwise), and only then issue the above interactive-eval command. Thus the eval command should have worked, cider already indicated that we got a cljs repl, why is it failing then?

The second change this patch introduces is to enable nREPL message logging and dump all buffers on test failure, so that the author can get more context on why things might have failed. Looking now at the nREPL tracing log produced, we can observe that the nREPL server is still reporting that the repl-type is clj but the test still went ahead and evaluated the probe command:

(<--
               id                 "8"
               session            "e682183a-d558-46fb-96f0-50c065fa5f64"
               time-stamp         "2022-12-09 07:28:38.232971000"
               changed-namespaces (dict)
               repl-type          "clj"
               status             ("state")
             )
             (-->
               id         "9"
               op         "eval"
               session    "e19e42a0-ff54-4cd4-a8cf-104240c5c6d7"
               time-stamp "2022-12-09 07:28:38.262420000"
               code       "(do (require '[shadow.cljs.devtools.api :as shadow]) (shadow..."
               ns         "shadow.user"
             )
             (-->
               id         "10"
               op         "eval"
               session    "e19e42a0-ff54-4cd4-a8cf-104240c5c6d7"
               time-stamp "2022-12-09 07:28:40.276563000"
               code       "(print :cljs? (some? *clojurescript-version*))"
               ns         "shadow.user"
             )

This suggests that (cider-repls 'cljs nil) has falsy reported that there is a cljs repl available, but is not (it is still a clj repl). I haven't look at #3282 yet, but does it force CIDER to believe that the REPL is a cljs repl while it has not switched in reality yet from clj to cljs?

Nevertheless, could you please consider merging the improved diagnostics. Thanks

• The commits are consistent with our contribution guidelines
  - [ ] You've added tests (if possible) to cover your change(s)
• All tests are passing (eldev test)
• All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
    - [ ] You've updated the changelog (if adding/changing user-visible functionality)
    - [ ] You've updated the user manual (if adding/changing user-visible functionality)

[ikappaki]

(I've also pushed another commit replicating the commentary of the basic bb test case to all other jack in tests so to aid with readability'; the nrepl-error buffer failure check at the end of each test -- and its parent unwind-protect -- has also been removed, since this is now covered by with-cider-test-sandbox i.e. when there is a failure all buffers, includes that, are printed out)

[benjamin-asdf]

Is there still something I can do to help? Now that #3282 has a fix.

[ikappaki]

Is there still something I can do to help? Now that #3282 has a fix.

Hi @benjamin-asdf, I've created #3287 to describe the issue, it appears to me that the latest change has caused a regression by prematurely reporting the shadow repl as being cljs while it is not, causing a chain reaction to other parts of the code.

[ikappaki]

Hi @bbatsov, do you think you could review this PR please? It provides improved diagnostics messages when an integration test fails + additional commentary on the test cases replicating the common comments from the bb test. Thanks

[bbatsov]

Looks solid to me. Thanks!