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

Add new Alcotest.suite function to make writing tests easier #393

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

yawaramin
Copy link

Fixes #126.

Example usage:

{[
let () =
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ocamlformat settings don't like the formatting of this example. IMHO, the format settings are incorrect :-)

And update Lwt suite example.
@samoht
Copy link
Member

samoht commented Sep 20, 2023

Many thanks for your patch - two remarks:

  • I really like using functions in the API instead of exposing concrete types. It's probably too late to make those types abstract as it will break every existing code, but let's start by exposing proper constructors first and see how adoption goes.
  • I am not convinced by inlining all the test cases in "heavy" functions. Would you mind doing the minimum possible changes in the examples to keep the test functions separate (when they are big enough like they are right now) and avoid the heavyweight begin fun () -> ... end forms when possible?

@yawaramin
Copy link
Author

yawaramin commented Sep 20, 2023

Hi @samoht thanks for the review. Regarding your points,

  • Agreed
  • Do you mean like e.g.:
let string_case_lower_case () =
  Alcotest.(check string) "same string" "hello!" (To_test.lowercase "hELLO!")

let string_case_capitalization () =
  Alcotest.(check string) "same string" "World." (To_test.capitalize "world.")

let string_concat_string_mashing () =
  Alcotest.(check string) "same string" "foobar" (To_test.str_concat ["foo"; "bar"])

let list_concat_list_mashing () =
  Alcotest.(check (list int)) "same lists" [1; 2; 3] (To_test.list_concat [1] [2; 3])

let () =
  Alcotest.suite "Utils" begin fun group ->
    group "string-case" begin fun case ->
      case "Lower case" string_case_lower_case;
      case "Capitalization" string_case_capitalization;
    end;

    group "string-concat" begin fun case ->
      case "String mashing" string_concat_string_mashing;
    end;

    group "list-concat" begin fun case ->
      case ~speed:`Slow "List mashing" list_concat_list_mashing;
    end;
  end

Wouldn't this bring us back to the current situation where the test implementations and registrations are separate and can get out of sync? E.g. if I don't have an empty .mli and delete one of the cases, I could forget to delete its implementation.

EDIT: also this means we need to decide on names for the test case functions, and they have to be reasonable and meaningful names. We can't just do e.g. let f1 () = .... So we are very likely saving quite a bit of naming decision-making and verbosity by just inlining the lambdas as fun () -> .... I think that's a win for users.

@lessp
Copy link

lessp commented Jun 29, 2024

Love this! Can we expect this to go in?

@samoht
Copy link
Member

samoht commented Mar 13, 2025

I'm looking at this again - I think there are 2 things in that patch that needs to be reviewed separately

  1. adding top-level types and function for group of tests. This is uncontroversial a good change and I'll be happy to merge this (given with find the right name, for instance group_testlist is a bit weird :p)
  2. changing the default tests are registered -- I'm a bit warry of this one without more user feedback. I'm ok to suggest a new way to do things but I would like to avoid breaking existing tests. Once users start to adopt this new way (and give us some feedback) we could consider changing the way we write examples, etc.

Do you think that makes sense?

@samoht
Copy link
Member

samoht commented Mar 13, 2025

In particular, I'd like to make sure we take the feedback from #294 into account

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.

Review how tests are registered
3 participants