Skip to content

Flambda2 test list#494

Merged
mshinwell merged 6 commits intooxcaml:mainfrom
xclerc:flambda2-test-list
Jan 31, 2022
Merged

Flambda2 test list#494
mshinwell merged 6 commits intooxcaml:mainfrom
xclerc:flambda2-test-list

Conversation

@xclerc
Copy link
Copy Markdown
Contributor

@xclerc xclerc commented Jan 31, 2022 •
edited
Loading

This pull request changes the logic behind the file named
"testsuite/flambda2-test-list" in order to explicitly exclude
test directories rather than explicitly include such directories.
We expect its maintenance to be easier that way, and in
particular there is no risks to forget to add a newly-created
directory.

This pull request is possibly best reviewed commit-by-commit:

  • commits from d32e541 to b1aac79 (both inclusive) are
    simply a minor clean-up, leaving the semantics of the file
    unchanged;
  • commit 1b89d07 changes the logic from opt-in to opt-out.

In order to double check that the last commit behaves as
expected, I diffed _runtest/flambda2-test-list before/after,
and got:

<   tests/comprehensions
---
>   tests/comprehension
109d108
<   tests/runtime-naked-pointers
110a110
>   tests/runtime-naked-pointers

which basically means that (i) the aforementioned clean-up
was not perfect and (ii) no tests were lost in the process.

@xclerc xclerc added flambda2 Prerequisite for, or part of, flambda2 CI Github Actions CI changes labels Jan 31, 2022
@Gbury
Copy link
Copy Markdown
Contributor

Gbury commented Jan 31, 2022

Maybe we should switch to using ocamlformat's features to enable/disable tests when running with flambda, rather than the current list of folders ?

@mshinwell
Copy link
Copy Markdown
Collaborator

mshinwell commented Jan 31, 2022

In due course perhaps (I presume you mean dune not ocamlformat?). One step at a time...

@Gbury
Copy link
Copy Markdown
Contributor

Gbury commented Jan 31, 2022 •
edited
Loading

In due course perhaps (I presume you mean dune not ocamlformat?). One step at a time...

Oups, I meant ocamltest: the current list of test folders to run is used with the upstream testsuite, which uses ocamltest, where one can disable tests by adding a few lines in the test definition at the top of the test file.

@xclerc
Copy link
Copy Markdown
Contributor Author

xclerc commented Jan 31, 2022

I certainly agree that using ocamltest builtin mechanism
would be better in the long run, but our immediate concern
was to not ignore new test directories. Also, this switch from
opt-in to opt-out while still going through a centralized file
has one (unexpected) benefit: it is basically a TODO list of
the tests we need to fix...

@xclerc xclerc mentioned this pull request Jan 31, 2022
Open
@xclerc xclerc marked this pull request as ready for review January 31, 2022 16:15
@xclerc xclerc requested a review from mshinwell as a code owner January 31, 2022 16:15
@mshinwell mshinwell merged commit 64f6ad7 into oxcaml:main Jan 31, 2022
mshinwell pushed a commit that referenced this pull request Feb 1, 2022
@xclerc @mshinwell
Flambda2 test list (#494)
d1cb194
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshinwell mshinwell mshinwell approved these changes

Assignees

No one assigned

Labels

CI Github Actions CI changes flambda2 Prerequisite for, or part of, flambda2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xclerc @Gbury @mshinwell