Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Remove ppx_expect dependency #43

Merged
merged 21 commits into from
Jul 23, 2019
Merged

Remove ppx_expect dependency #43

merged 21 commits into from
Jul 23, 2019

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jul 22, 2019 •
edited
Loading

ppx_expect is in theory usable in Mirage since it strips the code in the released binaries; but for now it causes some cross-compilation issues because of C stubs that rely on POSIX APIs. So for now let's do without it!

See janestreet/ppx_expect#15

@emillon
Copy link
Collaborator Author

emillon commented Jul 23, 2019

All right, this is ready for review. The diff is a bit daunting, but I've separated in roughly 3 phases of commits:

  • define a "test" library with all the expectations, and expose the extra values required in a For_tests module. This needs to be a library because executables can not have inline tests.
  • move the tested boundary to the public API of the main library. For example, port dh and scalar_mult tests to use key_exchange. This empties For_tests. This step also removes some now unnecessary tests (Point.of_hex can be removed for example).
  • port the tests to alcotest

Overall, this makes the library more specific to DHE but also makes it simpler. And removes the extra dependency!

@emillon emillon marked this pull request as ready for review July 23, 2019 07:53
@emillon emillon requested a review from NathanReb July 23, 2019 07:53
@emillon
Copy link
Collaborator Author

emillon commented Jul 23, 2019

Boom 💥! Now rebased.

NathanReb
NathanReb reviewed Jul 23, 2019
View reviewed changes
Copy link
Member

@NathanReb NathanReb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! I have a few minor comments about the tests but this is a welcome change!

Slightly unrelated to this PR but I added a test/ dir in #44. Might be worth moving all tests there under test/wycheproof/, test/p256/ and eventually test/functional/ if we wish to seperate them from the strictly speaking unit ones. Can be done in a separate PR ofc!

test/test_fiat_p256.ml Outdated Show resolved Hide resolved
test/test_fiat_p256.ml Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator Author

emillon commented Jul 23, 2019

I moved the tests hierarchy under test/ in order to have fewer things in the same place. That's a good idea!

NathanReb
NathanReb reviewed Jul 23, 2019
View reviewed changes
test/unit/test_p256.ml Outdated Show resolved Hide resolved
@emillon emillon self-assigned this Jul 23, 2019
@emillon
Copy link
Collaborator Author

emillon commented Jul 23, 2019

Thanks!

@emillon emillon merged commit a203800 into mirage:master Jul 23, 2019
@emillon emillon deleted the no-ppx-expect branch July 23, 2019 10:00
@emillon
Copy link
Collaborator Author

emillon commented Jul 23, 2019

(I forgot to push the last fix with ids - did it on master 🙀)

@emillon emillon mentioned this pull request Jul 23, 2019
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 23, 2019
@emillon
[new release] fiat-p256 (0.2.0)
045b015 
CHANGES:

*2019-07-23*

### Fixed

- Fix a bug in `generate_key` where it would never actually work when used with
  a proper `rng` function (mirage/fiat#44, @NathanReb)
- Fix benchmark executable. It is now built (but not executed) as part of tests
  (mirage/fiat#45, @emillon)

### Changed

- Use `alcotest` instead of `ppx_expect` for tests (mirage/fiat#43, @emillon)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NathanReb NathanReb NathanReb left review comments

Assignees

@emillon emillon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emillon @NathanReb