Please consider adapting websocket-functional-test.el to use ert-deftest

[sten0]

Hi,

Thanks again for maintaining this software. As mentioned in another issue I'm investigating packaging this library for Debian. Would you please consider adapting websocket-functional-test.el to use ert-deftest? I maintain the Debian package for Elpy, so I know it's possible ;-) As for the "why" our Emacsen Team exclusively uses ERTs for QA and CI.

Sincerely,
Nicholas

[ahyatt]

I can use ert within the test, but do you also want it to not be run from the command line to execute?

[sten0]

I've attached a build log for reference.
emacs-websocket_1.12+1.g74e4b82-1_amd64-2019-11-24T01:05:53Z.build.zip

The relevant section is L733 to L796, and L734 is where we run ert-run-tests-batch-and-exit to test the package as-installed, with the variables isolated to only the package that is being tested. For the purposes of the Debian package it's enough to call testserver.py in the appropriate ERT test unit, and/or fork a process that multiple units can test against. Of course you're welcome to use any other additional test runners :-) (eg: may projects have a "test" target in a Makefile or use Cask, but we ignore those)

It looks like websocket-server-close, websocket-to-bytes, and maybe others also need to be more strict, because it seems like these units should fail. Additionally I'm surprised that the error output isn't attached to an error-handling test unit. If it's an async process, maybe a test unit later in the series could catch the output?

Finally, it would be nice to move away from the obsolete flet macro, to either ‘cl-flet’ or ‘cl-letf’. That's "wishlist" priority though, I don't want to be a PITA!

P.S. There's already been an uncharacteristically early positive response to my ITP (intent to package email), so I think you'll appreciate knowing that people are excited about seeing your work included in Debian :-)

[sten0]

P.S. what I mean by "error-handling" test unit is a test unit designed to test the libraries error-handling capabilities. eg: test for expected failure error string, and pass if it matches or fail if the error string isn't triggered when the failure case is triggered.

[ahyatt]

Can you comment further on why websocket-server-close and websocket-to-bytes should fail? I'm not sure what you are referring to - the ert tests you are referring to work, because the code under test works. I took another look at the code but didn't see anything obviously wrong.

About error-handling tests, I don't personally believe it is all that useful to test actual error messages. We do test that the library does error out when it is supposed to.

[sten0]

Hi Andrew, Andrew Hyatt <notifications@github.com> writes:

Can you comment further on why `websocket-server-close` and websocket-to-bytes` should fail? I'm not sure what you are referring to - the ert tests you are referring to work, because the code under test works. I took another look at the code but didn't see anything obviously wrong.

testserver.py is not called, and python isn't installed (see previously attached build log), so I expect some self tests to fail, because testserver.py + ERT tests seem to exist to verify that communication over a websocket functions correctly. The console output indicates that communication with the server is failing, and it's not clear which test unit this is attached to. Evidence of the failure is in the unhandled error output: ``` Websocket client connection: Host header not found ... Websocket client connection: HTTP/1.1 not found ```

About error-handling tests, I don't personally believe it is all that useful to test actual error messages. We do test that the library does error out when it is supposed to.

Fair point :-) one can test for success instead, and of course it doesn't need to be a string comparison--that was just a example. The issue is that tests always pass, even when there a server is not active, and thus when all websocket transfers will fail. If tests always pass, even when the server is absent, then the results have superficial value. eg: The units and associated code-paths have valid syntax and no logic errors, but aren't actually testing functionality. I'm sorry I won't have the free time to jump in and work on fixing them myself until mid January (at the earliest), and I understand it's annoying and/or insulting when people complain but don't contribute code. This bug report is a request to call testserver.py as part of the ERT tests and to integrate `websocket-functional-test.el` into those tests. Here are some units that seem like they should fail when the server is not running, and thus when websocket transfers necessarily fail (there may be more): websocket-get-server-response, websocket-send, websocket-send-text, websocket-server-close, websocket-verify-response-code Best, Nicholas

[ahyatt]

I think there may be some confusion here. There's two main testing files in the websocket package: the websocket-functional-test.el file and the websocket-test.el. The unit tests you mention are in websocket-test.el, and, like all the ert tests in the package, are true unit test, not dependent on anything being set up outside of the tests themselves. This is why they mock out functionality with flet (which I agree should be cleaned up eventually). This is why they pass without a Tornado server being run, because they were never written to require one.

Please let me know if I'm misunderstanding your point.

[ahyatt]

PTAL at commit 69ee80a and let me know if it seems acceptable to you. It should fulfill the requirements as described.

[ahyatt]

I've merged this into the master branch now. Thank you for the suggestion!

[sten0]

@ahyatt, sorry for the delay replying; I got lost in the backlog of work and messages. Thank you very much for working on this! It looks good, with one issue:
The new method does not work on standalone systems because it depends on connectivity to echo.websocket.org, eg:
https://tests.reproducible-builds.org/debian/rb-pkg/unstable/amd64/emacs-websocket.html and search for "Test websocket-client-with-remote-server backtrace"

I'll understand if this isn't something you're interested in supporting ;-) It would be nice to have though, since the alternative is disabling this test on Debian infrastructure.

Thank you!
Nicholas

[ahyatt]

I think it's valuable to have functional tests that use an outside source of truth such as echo.websocket.org. Disabling the functional tests in Debian seems like a reasonable idea. We do the opposite in the emacs elpa repository - we don't include the unit tests, since there have been contributions there I have authors that haven't signed the FSF papers.

[sten0]

Sorry for the delay again; it took a while to find the time to look into this!

I think it's valuable to have functional tests that use an outside source of truth

Agreed :-)

such as echo.websocket.org.

Unless one is testing DNS or routing, I don't see how this is more valuable than a server (that is independent of emacs-websocket) running on localhost. For example, after reading up on what other software uses to validate its websocket support I found that the Autobahn|Testsuite seems to the standard by which all others are measured. For the purposes of CI, it's available as the python3-autobahn package as early as Ubuntu bionic (18.04LTS) and Debian stretch (Debian 9). There are other server implementations that declare compliance with this testsuite that may be more convenient to use, but I haven't looked into them in any kind of depth.

Disabling the functional tests in Debian seems like a reasonable idea.

Pulling out teeth also seems like a reasonable idea ;-) and I think we'll agree that context and consideration of the available alternatives is important.

We do the opposite in the emacs elpa repository - we don't include the unit tests, since there have been contributions there I have authors that haven't signed the FSF papers.

If an author has disappeared, has refused to sign the papers, objects to the FSF's invasive identity verification, or the concept of copyright assignment to a 3rd party, then no other options exist (other than a clean-room rewrite by someone who hasn't seen the existing unit tests...but that's pedantic, no?).

If you don't want to work on this, would you accept a PR that enables the use of an alternative "outside source of truth" such as Autobahn running on localhost? I imagine it will require these: a variable for the URL; a variable for a "server-start-command" and another for "server-kill-command"; possibly a third variable to hold the server's PID; functions to make use of these, plus a branch to take the "server-start-command" path rather than the echo.websocket.org one if the "websocket-test-url" variable is set. Everything apart from the URL allows Emacs to manage the server--to minimise dependencies on external frameworks.

Thanks,
Nicholas

[ahyatt]

I previously used Python's Tornado webserver to test out websocket in our functional test. Yes, it was nice to have something purely on-machine, but I found the disadvantages outweighed the advantages, the main disadvantage being the user needed to have something installed to test with. Installing things is not always so easy. For most manual runs of the functional tests, the dependency was a source of failures and didn't buy a lot.

However, if you wanted to set up some sort of continuous functional test using Autobahn because this is useful for Debian, that's fine with me. Send over a PR and I will take a look.

One idea I never implemented is to use the emacs websocket client to talk to the server. Benefit: no outside dependencies at all. Drawback: you can't really be sure you are testing anything.