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

Revive tests and improve the tests runner #25

Merged
merged 14 commits into from
May 6, 2023
Merged

Revive tests and improve the tests runner #25

merged 14 commits into from
May 6, 2023

Conversation

Niols
Copy link
Member

@Niols Niols commented May 5, 2023

This PR is here to bring back the tests that we have (and that aren't that much to begin with). There will be several parts to this:

  • Update the CI so that it runs tests again.
  • Update the Dune configuration so that dune test does what we expect.
  • Update the Nix build so that it also runs the tests.
  • Fix the test runner to follow changes in Morbig.

Edit: mentioned later in the PR:

  • The comparison of ASTs should be made without comparing the locations. (This is maybe already the case but it should then be documented.) Edit: Introduce an AST equality that ignore the locations #31
  • The printing of ASTs in JSON should hide the locations in the test runner. In general, we should provide two different serialisers, one with and one without locations. Alternatively, we could rework Morsmall to provide two ASTs, one with and one without locations; this should be easy to do with a well-made functor. Edit: Introduce a JSON serialisation that ignores locations #32
  • Currently, in CI, we get the logging output of the tests runner, but this does not include the artifacts. We should upload them to allow inspection. Edit: CI should update the test artifacts #33

@Niols Niols self-assigned this May 5, 2023
@Niols
Copy link
Member Author

Niols commented May 5, 2023

Yay, the CI is now failing all over the place \o

image

@Niols
Copy link
Member Author

Niols commented May 5, 2023

The test runner is now looking much better. It compiles, the code is cleaner and its error reporting is also much better. Still, a few things remain before we can do anything with it:

  • The comparison of ASTs should be made without comparing the locations. (This is maybe already the case but it should then be documented.)
  • The printing of ASTs in JSON should hide the locations in the test runner. In general, we should provide two different serialisers, one with and one without locations. Alternatively, we could rework Morsmall to provide two ASTs, one with and one without locations; this should be easy to do with a well-made functor.

@Niols
Copy link
Member Author

Niols commented May 5, 2023

Additional thought:

  • Currently, in CI, we get the logging output of the tests runner, but this does not include the artifacts. We should upload them to allow inspection.

@Niols
Copy link
Member Author

Niols commented May 6, 2023

I think that, at this point, we can consider that the tests are fixed indeed. CI is unpleased because Morsmall is broken in many ways, but that is not so much the problem of the tests. We do need to fix these last three points but I will open issues for all of them before merging this PR.

@Niols Niols changed the title Fix tests Revive tests and improve the tests runner May 6, 2023
@Niols Niols marked this pull request as ready for review May 6, 2023 11:48
@Niols Niols merged commit fe6ce1b into main May 6, 2023
@Niols Niols deleted the fix-tests branch May 6, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant