-
Notifications
You must be signed in to change notification settings - Fork 99
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
Use mops
to run tests
#579
Conversation
LGTM; thanks! I tested locally, and this is fantastic. |
test/Makefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crusso, would you prefer to keep the original Makefile in addition to the updated testing system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes, though I haven't got the bandwith to think about just now. Any reason it's being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason it's being removed?
I noticed that it was marked as deprecated, and assumed that it was not being used(also didn't find usage in CI files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think its in use the motoko-base CI, but is used from the motoko CI. Perhaps we can fix that somehow in the future, but not at the moment. Is it a problem to just keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dfinity/motoko/blob/4646d1219d08c7223f82f82da52d4c196c908658/default.nix#L651 is the line where we use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will return these files.
It's worth mentioning that the unit tests originally ran in both the interpreter and Wasmtime (except for tests with the explicit |
Looks cool, but haven't had a change to try it (and won't before leaving on holiday for 1.5 wks). Is there any reason to remove the makefile and vessel dependencies. Come to think of it, I actually think the former will break Motoko CI since we run the base tests as part of CI, and almost certainly use the Makefile for that. @ggreif amirite? |
Co-authored-by: Ryan Vandersmith <ryanvandersmith@gmail.com>
Option to run all tests in wasi mode? Yes, I can add this feature to mops. |
Is there probability to get false positive results depending on if tests are run in interpreter mode or in wasi mode? |
The WASI and interpreter runtimes work slightly differently, so it's possible that a bug in the interpreter might cause a test to fail where it works fine in the WASI runtime, and vice versa. This has actually happened in the past, so it's helpful to run the base library tests using both WASI and the interpreter whenever possible (although this is perhaps a unique requirement for the base library compared to most other Motoko projects). |
Returned vessel's files and makefile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving (will give the rest of the team a chance to weigh in before merging).
mops test
.vessel
dependency.// @testmode wasi
comment line will be compiled and run with wasmtime.Mops can run tests in parallel, results:
On my laptop: 15s vs 50s
In GitHub CI: 30s vs 1m 33s