Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port lwttester to Alcotest #935

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Feb 24, 2022

I was writing some tests for Lwt and got slightly annoyed at Lwt's
ad-hoc testing engine, which I'm sure is battle-proof but suffers a bit
from an usability standpoint, so I ported it to Alcotest.

Alcotest is "a lightweight and colourful test framework".

It provides benefits over Lwt's current lwttester: it allows selecting
which test cases and test suites to run, has a colorful logging, better
integration with dune and standalone testing, and better logging
overall.

Porting the whole test suite was deemed too complex without
code-rewriting tools, so only Lwt's Test library was re-implemented on
top of Alcotest. We can revisit that later.

  1. The main caveat is that Alcotest has no support for running tests
    concurrently, so the whole run is slightly longer.
  2. Another problem is that alcotest-lwt re-exports lwt.unix which causes
    circular dependencies, so version alcotest-lwt.1.5.0 was embedded.
  3. Usage of the Skip exception to skip a test while it's running cannot
    be ported to Alcotest too.
  4. Alcotest requires OCaml >= 4.05. We keep the old test suite for
    OCaml < 4.05.

Gentle cc to @craigfe @aantron @raphael-proust :)

Links:

@MisterDA MisterDA force-pushed the alcotest branch 5 times, most recently from 95967c8 to 4d507c0 Compare February 25, 2022 14:07
@MisterDA MisterDA marked this pull request as draft February 25, 2022 14:15
[Alcotest][] is "a lightweight and colourful test framework".

It provides benefits over Lwt's current lwttester: it allows selecting
which test cases and test suites to run, has a colorful logging, better
integration with dune and standalone testing, and better logging
overall.

Porting the whole test suite was deemed too complex without
code-rewriting tools, so only Lwt's Test library was re-implemented on
top of Alcotest. We can revisit that later.

1. The main caveat is that Alcotest has no support for running tests
   concurrently, so the whole run is slightly longer.
2. Another problem is that alcotest-lwt re-exports lwt.unix which causes
   circular dependencies, so version alcotest-lwt.1.5.0 was embedded.
3. Usage of the Skip exception to skip a test while it's running cannot
   be ported to Alcotest too.
4. Alcotest requires OCaml >= 4.05. We keep the old test suite for
   OCaml < 4.05.
@raphael-proust
Copy link
Collaborator

What does that do to the cone of dependency of Lwt? What does alcotest depend on?

I'm quite reluctant to add dependencies to Lwt in general. I'm ok with some exceptions but Lwt needs to stay a leaf-dependency as much as possible. This might be ok on the ground that it's a test-only dependency. But still I want to understand that a bit more.

Is it possible to vendor alcotest instead?
Or maybe patching parts of lwttester to make it more like alcotest?

What do you mean by "standalone testing"?

@MisterDA
Copy link
Contributor Author

MisterDA commented Mar 3, 2022

What does that do to the cone of dependency of Lwt? What does alcotest depend on?

I'm quite reluctant to add dependencies to Lwt in general. I'm ok with some exceptions but Lwt needs to stay a leaf-dependency as much as possible. This might be ok on the ground that it's a test-only dependency. But still I want to understand that a bit more.

As you've guessed, Lwt's dependencies don't change, it's only the test dependencies. Alcotest 1.5.0 depends on these packages:

  "dune" {>= "2.8"}
  "ocaml" {>= "4.05.0"}
  "fmt" {>= "0.8.7"}
  "astring"
  "cmdliner" {>= "1.0.0" & < "1.1.0"}
  "re" {>= "1.7.2"}
  "stdlib-shims"
  "uutf" {>= "1.0.1"}
  "ocaml-syntax-shims"

relatively standard.

Is it possible to vendor alcotest instead? Or maybe patching parts of lwttester to make it more like alcotest?

Although Alcotest has a small API, it's not small by itself... lwttester API is already close to Alcotest's, which allowed me to port lwttester on top of Alcotest.
If we were to modify lwttester instead, my pain points were that I couldn't select which tests I wanted to run, and that it was difficult to locate the test logs (especially on Windows).

What do you mean by "standalone testing"?

I meant running a specific test, or a set of tests while working on Lwt, which I think Alcotest is better at than lwttester. When simply running dune runtest @all both are quite comparable.

Alcotest still has the major caveat for now that it can't run tests concurrently, which lengthens the run time of the test suite. This has been discussed before, and there's an issue tracking that problem.
I haven't found a way of ignoring alcotest and falling back to lwttester on OCaml < 4.05. I've been suggested to use a dune env/profile/workspace to make lwttester and alcotest exist alongside each other, but haven't explored that yet.

@raphael-proust
Copy link
Collaborator

"dune" {>= "2.8"}
"ocaml" {>= "4.05.0"}
"fmt" {>= "0.8.7"}
"astring"
"cmdliner" {>= "1.0.0" & < "1.1.0"}
"re" {>= "1.7.2"}
"stdlib-shims"
"uutf" {>= "1.0.1"}
"ocaml-syntax-shims"

This is not too extravagant but it is still significant. I'm worried that we will get to a point where we can't run tests on this compiler variant/version, or we can't run tests for lwt_ppx, or some other such scenario, because of some incompatibilities. In a way, this is already the case with <4.05.

Anyone else has opinions on the matter?

@aantron
Copy link
Collaborator

aantron commented Mar 6, 2022

Alcotest still has the major caveat for now that it can't run tests concurrently, which lengthens the run time of the test suite. This has been discussed before, and there's an issue tracking that problem.

This is the main reason I did not port Lwt's tester to Alcotest, but opened mirage/alcotest#177. I'm not sure if you've ported the whole test suite to Alcotest yet, but if you do, you should find it that the tests take considerably longer to run if not run concurrently. This is a major obstacle for local development, when running tests on your own machine. I still recommend not porting the test suite to Alcotest until Alcotest can run tests concurrently.

@aantron
Copy link
Collaborator

aantron commented Mar 6, 2022

From #712 ("Run Lwt_unix tests concurrently"):

This decreases testing time for Lwt_unix from about 47 seconds to about 6 seconds, and the overall testing time is reduced from 53 seconds to 12 seconds (all measurements on my system).

Running tests concurrently is practically necessary if more of #539 ("Test the Unix binding") is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants