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

Introduce a versioned API for Alcotest V1 #306

Merged
merged 1 commit into from
May 10, 2021

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented May 6, 2021

This is the first real step towards #294: introducing a V1 namespace with a stronger stability guarantee. The Alcotest.V1 module is simply an alias of the current Alcotest API, enabling users to get fast migrations to future major releases by aliasing it:

module Alcotest = Alcotest.V1

@craigfe craigfe force-pushed the versioned-api branch 3 times, most recently from 72372f5 to b7e8120 Compare May 6, 2021 23:25
@craigfe craigfe force-pushed the versioned-api branch 2 times, most recently from a94c71b to 1aaa46d Compare May 7, 2021 13:47
@hannesm
Copy link
Member

hannesm commented May 16, 2021

I am really not convinced of this approach (and indeed, some other libraries in the mirage organisation adapted such a behaviour already). From a zoomed out perspective, the packaging system (opam) is responsible for versioning. With an alcotest move from 1.x to 2.y, since it is a major version bump, the API may be revised, and clients have to adapt.

Now, introducing V1 modules in alcotest 2.y mean:

  • we'll have to maintain them (or figure out: when will they be deprecated / disappear) -- leading to code smell/duplications,
  • clients need to adapt to the new API anyways (at least by using the V1 modules) -> in opam, there will be lots of upper bounds and releases for alcotest 2.y compatibility.

So, what is the gain of such VZ modules? I really don't understand it. As a contributor and client of alcotest, I'm fine with adapting to a new API if a major version bump is done. For not actively maintained clients, they'll have an upper bound alcotest {< "2.0.0"} - which is fine, or is it not?

If you want smooth transitions from 1.x to 2.y, you should preserve the existing interface as is (no V1 prefix), and can still call it 1.x ;)

Some years back, we discussed how to deprecate functions, the resulting text is available at https://github.com/mirage/canopy-data/blob/master/Posts/Deprecating -- not sure whether this is still up to date.

After all, I'm wondering what the long-term plan for "versioned API for Alcotest V1" is, is there any plan to ever remove that module? I think the conduit API moved in that direction without any clear idea how to remove any of the code.

@craigfe
Copy link
Member Author

craigfe commented May 16, 2021

I agree that this approach doesn't have any benefits for old projects that are not actively developed: they can just add an upper bound indefinitely (and this remains true when providing additional V1, V2 ... modules). It's also true that it doesn't solve the problem of needing to add these – at least temporarily – when releasing V2 to opam-repository.

The motivation for versioned submodules is that they make it feasible to gradually adopt new features in large – but still active – codebases for which a total migration is not worthwhile. New features can provide value to new users in terms of flexibility of test structuring / more expressive test predicates etc., but this isn't useful to test suites for which the user has already structured things exactly how Alcotest V1 requires.

(From my own work, I'm thinking of Tezos and Irmin, which still have many new tests added but for which there's no value to going back and spending hours changing the already-existing boilerplate to be a slightly different shape. Incidentally, Irmin also provides V1 modules for use in Tezos for the same reason.)

From a zoomed out perspective, the packaging system (opam) is responsible for versioning.

Versioning large changes with Opam alone forces users into all-or-nothing adoption in each of their repositories. This seems sensible to me when the large alters behaviour fundamentally or adds a lot of value to existing call-sites, but I think neither of these hold here:

  • the current test DSL is tiny and should be fairly easy to provide compatibility for. (My current V2 draft implements the migration in < 100 lines of mostly types here.)
  • the value added by new features will never justify a complete migration in a sufficiently-large codebase, so such users will just stick to Alcotest V1 forever (and I'd end up needing to maintain a V1 branch for these projects anyway).

All in all, I think the fact that Opam doesn't support installing multiple versions of the same package (unlike, e.g. npm's package aliases) means that it can never be the all-encompassing solution for versioning in large codebases.

Some years back, we discussed how to deprecate functions, the resulting text is available at https://github.com/mirage/canopy-data/blob/master/Posts/Deprecating -- not sure whether this is still up to date.

I like these guidelines as they apply to small API changes like individual function removals, under the assumption that all existing users will eventually want to migrate unconditionally to the "new way" across their entire dependency tree. However, API changes can be worthwhile / feasible for new users but not existing ones. (Another example of this is encoding formats in Irmin, which some users will inherently leak as part of their own API and so need an internal versioning scheme.)

After all, I'm wondering what the long-term plan for "versioned API for Alcotest V1" is, is there any plan to ever remove that module? I think the conduit API moved in that direction without any clear idea how to remove any of the code.

I suspect Alcotest V1 is simple enough to be maintained indefinitely, but it's also possible to do a deprecate / remove cycle several major versions down the line. The span of Va ... Vz modules supported by Alcotest main need reflect only what is worthwhile to maintain at the time. Viewed this way, the point of these submodules is to provide deprecated namespaces that don't get in the way of new APIs.

Hope that clarifies my justification; apologies for the length :-)

craigfe added a commit to craigfe/opam-repository that referenced this pull request Oct 9, 2021
…and alcotest-lwt (1.5.0)

CHANGES:

- Make Alcotest compatible with `js_of_ocaml.3.11.0`. Users can depend on the
  new virtual `alcotest-js` Opam library to pick up the right `js_of_ocaml`
  version automatically. (mirage/alcotest#326 mirage/alcotest#328, @hhugo @smorimoto)

- Record exception backtraces during test suite runs by default. This behaviour
  can be disabled by passing `~record_backtrace:false` to `Alcotest.run`. (mirage/alcotest#317,
  @craigfe)

- Generate shorter unique identifiers for test runs (8-character alphanumeric,
  rather than a full 128-bit UUID). (mirage/alcotest#304, @craigfe)

- Change `Alcotest.{char,string}` pretty-printers to use OCaml syntax on
  assertion failures (i.e. wrap with quotes and escape control characters).
  (mirage/alcotest#318, @craigfe)

- Fix process for getting the width of attached terminals on MacOS.
  Previously, a terminal width of 80 columns was assumed. (mirage/alcotest#325, @craigfe)

- Fix parsing of test filter ranges to allow '-' separators (e.g. `test alpha
  1-4`), as advertised in the manpage. The previously-used '..' separator is
  also supported. (mirage/alcotest#312, @craigfe)

- Introduce an `Alcotest.V1` module that aliases the existing `Alcotest` API and
  provides a stability guarantee over major version changes. Similar versioned
  aliases also exist for the backends: `Alcotest_{async,lwt}.V1`. (mirage/alcotest#306,
  @craigfe)

- Change the `~filter` argument to `Alcotest.run` to be a predicate over tests.
  (mirage/alcotest#305, @craigfe)

- Renamed / removed some less frequently used modules used by the test backends:
  - `Alcotest.Unix` -> `Alcotest.Unix_platform`
  - `Alcotest_engine.{Cli,Core,Test}` -> `Alcotest_engine.V1.{Cli,Core,Test}`
  - `Alcotest.{Cli,Core}` are now gone. Use `Alcotest_engine.V1.{Cli,Core}.Make
    (Alcotest.Unix_platform)` instead.
  (mirage/alcotest#306 mirage/alcotest#309, @craigfe)

- Avoid exporting `list_tests` in the main test APIs (`Alcotest{,_lwt,_async}`).
  Use `Alcotest_engine` directly if you want this function. (mirage/alcotest#310, @craigfe)
craigfe added a commit to craigfe/opam-repository that referenced this pull request Oct 9, 2021
…and alcotest-lwt (1.5.0)

CHANGES:

- Make Alcotest compatible with `js_of_ocaml.3.11.0`. Users can depend on the
  new virtual `alcotest-js` Opam library to pick up the right `js_of_ocaml`
  version automatically. (mirage/alcotest#326 mirage/alcotest#328, @hhugo @smorimoto)

- Record exception backtraces during test suite runs by default. This behaviour
  can be disabled by passing `~record_backtrace:false` to `Alcotest.run`. (mirage/alcotest#317,
  @craigfe)

- Generate shorter unique identifiers for test runs (8-character alphanumeric,
  rather than a full 128-bit UUID). (mirage/alcotest#304, @craigfe)

- Change `Alcotest.{char,string}` pretty-printers to use OCaml syntax on
  assertion failures (i.e. wrap with quotes and escape control characters).
  (mirage/alcotest#318, @craigfe)

- Fix process for getting the width of attached terminals on MacOS.
  Previously, a terminal width of 80 columns was assumed. (mirage/alcotest#325, @craigfe)

- Fix parsing of test filter ranges to allow '-' separators (e.g. `test alpha
  1-4`), as advertised in the manpage. The previously-used '..' separator is
  also supported. (mirage/alcotest#312, @craigfe)

- Introduce an `Alcotest.V1` module that aliases the existing `Alcotest` API and
  provides a stability guarantee over major version changes. Similar versioned
  aliases also exist for the backends: `Alcotest_{async,lwt}.V1`. (mirage/alcotest#306,
  @craigfe)

- Change the `~filter` argument to `Alcotest.run` to be a predicate over tests.
  (mirage/alcotest#305, @craigfe)

- Renamed / removed some less frequently used modules used by the test backends:
  - `Alcotest.Unix` -> `Alcotest.Unix_platform`
  - `Alcotest_engine.{Cli,Core,Test}` -> `Alcotest_engine.V1.{Cli,Core,Test}`
  - `Alcotest.{Cli,Core}` are now gone. Use `Alcotest_engine.V1.{Cli,Core}.Make
    (Alcotest.Unix_platform)` instead.
  (mirage/alcotest#306 mirage/alcotest#309, @craigfe)

- Avoid exporting `list_tests` in the main test APIs (`Alcotest{,_lwt,_async}`).
  Use `Alcotest_engine` directly if you want this function. (mirage/alcotest#310, @craigfe)
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