Introduce integration tests

[ikappaki]

Hi,

can you please consider patch to introduce integration tests to CIDER. It addresses #3274.

Other than adding integration tests for jack-in covering the major project tools, it also fixes a long standing issue with nREPL leaving orphaned child processes on MS-Windows after being killed,. The issue is fixed by contracting the external taskkill MS-Windows program to do the job.

The integration tests appear to work consistently across all platforms, and have already discovered a bug with shadow reported under #3277, thus it will currently show as failing.

I haven't updated the changelog yet, waiting for some review feedback.

Detailed changelog in order of appears as follows:

1. Migrated macos regression tests to circle ci, but with a reduced set to only cover Emacs latest version.
2. Replaced existing macos only testing github action to run the integration tests across macos, ubuntu and windows latest on Emacs 28, 27 and 26. It was nearly impossible to do so on circleci across all platforms, thus a github action was chosen.
3. Updated the Eldev project file to support integration test via a new --test-type command line option (the template was taken from the Eldev tool's project file itself).
4. The cider server is given now a 0.5 sec opportunity to execute the close op before the server is killed.
5. The hacking section in documentation has been updated to mention integration tests.
6. The nrepl--kill-process has been updated to address the orphaned child process menace under MS-Windows, whereby the nREPL child processes were most likely to be left orphaned after the parent process was killed. The kill is now done by contracting the external taskkill windows program to do the job if available, and falls back to the previous logic otherwise.
7. The nrepl-server-sentinel has been simplified to only report an error if the nREPL process couldn't bootstrap itself, or send an exit message otherwise. The old code only closed client connections on hangup (i.e. HUP signal), but the new logic will close clients on any fatal signal. Not sure why the old code only did this on HUP, but it seems sensible to me that any open client connection should close when the server has exited.
8. Fixed an issue in cider-tests.el that a couple of buffer local shadow-cljs variables were set as global by mistake, thus affecting other tests (and in particular the integration tests).
9. Updated nrepl-client-tests.el to test the new nrepl plist property to indicate that the nREPL server has been successfully brought up, and also test that the new process-status exit returned on MS-Windows with taskkill is set.
10. Introduced integration tests covering jack-in for bb, clojure cli, lein and shadow.

• 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)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual extremely useful.

[bbatsov]

At first glance, everything looks pretty good to me. Might be a good idea to split the Windows fix to a separate commit, but whatever other feedback I have will likely be of cosmetic nature.

[ikappaki]

At first glance, everything looks pretty good to me. Might be a good idea to split the Windows fix to a separate commit, but whatever other feedback I have will likely be of cosmetic nature.

Yes, sorry, I've failed to mention that the windows child process fix is essential, otherwise the integration tests are intermittedly failing (especially the clojure cli jack in test) due to the incomplete kill.

[ikappaki]

Hi @bbatsov,

now that #3277 is fixed, I've added the following with the latest commit.

1. A changelog entry
2. An additional check to the each jack in test to wait for the client to connect to the server before proceeding further. This is needed because it has been observed that the client now sends asynchronous commands to the server to enquire about its capabilities. If the server response does not arrive before the client connection is closed, then the client handler waiting for the response raises an error. Waiting for the client to indicate succesfuly connection at the start (and thus receive the server capabilities) aliviates this issue.

All integration tests now pass, are you happy to proceed with the review please?

Thanks

[bbatsov]

Using ? is not common in Elisp - here it'd be is-connected for a variable and connected-p for a predicate.

[bbatsov]

Other than the small naming remark I made, everything looks reasonably good to me.

[ikappaki]

Other than the small naming remark I made, everything looks reasonably good to me.

Hi @bbatsov, I reilized that the lazy method I used to poll the client as to whether it has connected to the nREPL server was flawed (I was modifying the temp's buffer local hook var, which might not be in scope when the hook runs by the process filter), and decided to improve on that that by creating a new utility helper function that modifies the global cider-connected-hook, which now needs to be restored at the end of the test. I thus introduced another utility function called with-cider-test-sandbox to restore the application state on test exit. I've relocated/all all integration tests fns to the new integration-test-utils.el file, and added tests for these so we are sure they are all suppose to work as expected.

Can you please review the latest patch, the change log is as follows

1. A with-cider-test-sandbox macro is introduced to restore cider state after test has finished.
2. The cider-itu-poll-until helper function is introduced to replace nrepl-tests-sleep-until, this fn now throws if condition does not become true after the timeout has elapsed.
3. There was an issue with the previous attempt to add a hook and return a value for the test to poll when he client is connected to the nREPL. The hook was naively attached to the temp buffer, which is not necessarily in scope when the hook is run. A new cider-itu-nrepl-client-connected-ref-make! replaces that fn, which now returns a global variable ref, and has to be called inside with-cider-test-sandbox so that the hook variable is restored afterthe test finishes.
4. Introduced tests to test the new integration tests helper functions.
5. All integration tests are updated to use the new functions mentioned above.
6. Renamed connected? to is-connected as per review comments.
7. Restored nrepl-client-tests.el update which was overwritten with previous commit.

thanks

[bbatsov]

I love the most recent set of changes! Well done! While there's always some room for improvement I think we can merge them at this point and re-iterate on them as we go.

I'm curious to see them in action and in particular how stable will they be, as with integration tests of such complex software there's always the risk for some flakiness if we didn't account properly for something. Let's see!

[ikappaki]

I love the most recent set of changes! Well done! While there's always some room for improvement I think we can merge them at this point and re-iterate on them as we go.

I'm curious to see them in action and in particular how stable will they be, as with integration tests of such complex software there's always the risk for some flakiness if we didn't account properly for something. Let's see!

Thanks! happy to support and look after any potential issues that might come out of it, just let me know if something comes up :)