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?

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