-
Notifications
You must be signed in to change notification settings - Fork 707
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 tests for massa execution #2296
Conversation
177b8f7
to
1c09292
Compare
1c09292
to
3a1afe2
Compare
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.
LGTM, event if I would prefer to import wasm_tests
as a git submodule or an external dependency rather than adding it to this repo ...
bors merge |
2296: Add tests for massa execution r=adrien-zinger a=adrien-zinger Add tests for the execution module & fix events. Related with the issue #2079, we want to add test in the execution module. I suggest to not duplicate all tests, but test the layer of the massa usage. For example, we add the events and the current PR test if the implementation is good. Add a unit test for the event generation, build a local wasm project on massa-execution build. Then execute the test. Fixes: the events were not stored correctly in `final_events` Changes: Filter slot, include last slot. _this PR was a draft because we need to merge #2290 before_ Co-authored-by: Adrien Zinger <zinger.ad@gmail.com>
Build failed: |
18873b7
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.
Warning; this adds a major dependency for the whole build system (yarn + all the JS/AS libs), even when not building tests but just compiling the program !
Would it be possible to put that behind a feature that disables this dependency and the tests that depend on it when it's off ? And to make sure that just building the node doesn't require a yarn setup ?
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.
I still see the problem I mentionned in my previous review
2249: Speculative execution support r=damip a=damip # Intro This is a rewrite of the execution system following the spec we already agreed upon, as well as good practices and with the big refactoring constraints in mind. # Goals * [x] enable speculative execution * [x] get ready for ledger unification # Practices * [x] no async because it's not needed * [x] short functions (max 50 lines of code) * [x] no panics, unless described * crates split between worker and exports * [x] execution * [x] ledger: it will be refactored for on-disk storage, we will split it then => #2342 * thorough documentation * [x] function docs * [x] algorithm description * [x] file-level documentation * [x] crate-level documentation * [x] test exports * [x] unit and functional tests: to be added in a followup #2296 * [x] use genericity whenever possible * [x] clippy lints # Checklist * [x] implement speculative execution * [x] split execution into worker/exports crates * [x] create massa-ledger crate * [x] integrate execution and ledger into the existing program * [x] repair bootstrap tests * [x] repair consensus tests * [x] improve documentation * [x] try on labnet * [x] reactivate execution tests => will be done in a followup #2296 * [x] add specific tests => will be done in the followup #2296 Co-authored-by: Damir Vodenicarevic <damipator@gmail.com> Co-authored-by: damip <damipator@gmail.com>
When #2249 is merged, we can adapt this one |
2249: Speculative execution support r=damip a=damip # Intro This is a rewrite of the execution system following the spec we already agreed upon, as well as good practices and with the big refactoring constraints in mind. # Goals * [x] enable speculative execution * [x] get ready for ledger unification # Practices * [x] no async because it's not needed * [x] short functions (max 50 lines of code) * [x] no panics, unless described * crates split between worker and exports * [x] execution * [x] ledger: it will be refactored for on-disk storage, we will split it then => #2342 * thorough documentation * [x] function docs * [x] algorithm description * [x] file-level documentation * [x] crate-level documentation * [x] test exports * [x] unit and functional tests: to be added in a followup #2296 * [x] use genericity whenever possible * [x] clippy lints # Checklist * [x] implement speculative execution * [x] split execution into worker/exports crates * [x] create massa-ledger crate * [x] integrate execution and ledger into the existing program * [x] repair bootstrap tests * [x] repair consensus tests * [x] improve documentation * [x] try on labnet * [x] reactivate execution tests => will be done in a followup #2296 * [x] add specific tests => will be done in the followup #2296 Co-authored-by: Damir Vodenicarevic <damipator@gmail.com> Co-authored-by: damip <damipator@gmail.com>
2249: Speculative execution support r=damip a=damip # Intro This is a rewrite of the execution system following the spec we already agreed upon, as well as good practices and with the big refactoring constraints in mind. # Goals * [x] enable speculative execution * [x] get ready for ledger unification # Practices * [x] no async because it's not needed * [x] short functions (max 50 lines of code) * [x] no panics, unless described * crates split between worker and exports * [x] execution * [x] ledger: it will be refactored for on-disk storage, we will split it then => #2342 * thorough documentation * [x] function docs * [x] algorithm description * [x] file-level documentation * [x] crate-level documentation * [x] test exports * [x] unit and functional tests: to be added in a followup #2296 * [x] use genericity whenever possible * [x] clippy lints # Checklist * [x] implement speculative execution * [x] split execution into worker/exports crates * [x] create massa-ledger crate * [x] integrate execution and ledger into the existing program * [x] repair bootstrap tests * [x] repair consensus tests * [x] improve documentation * [x] try on labnet * [x] reactivate execution tests => will be done in a followup #2296 * [x] add specific tests => will be done in the followup #2296 Co-authored-by: Damir Vodenicarevic <damipator@gmail.com> Co-authored-by: damip <damipator@gmail.com>
2249: Speculative execution support r=damip a=damip # Intro This is a rewrite of the execution system following the spec we already agreed upon, as well as good practices and with the big refactoring constraints in mind. # Goals * [x] enable speculative execution * [x] get ready for ledger unification # Practices * [x] no async because it's not needed * [x] short functions (max 50 lines of code) * [x] no panics, unless described * crates split between worker and exports * [x] execution * [x] ledger: it will be refactored for on-disk storage, we will split it then => #2342 * thorough documentation * [x] function docs * [x] algorithm description * [x] file-level documentation * [x] crate-level documentation * [x] test exports * [x] unit and functional tests: to be added in a followup #2296 * [x] use genericity whenever possible * [x] clippy lints # Checklist * [x] implement speculative execution * [x] split execution into worker/exports crates * [x] create massa-ledger crate * [x] integrate execution and ledger into the existing program * [x] repair bootstrap tests * [x] repair consensus tests * [x] improve documentation * [x] try on labnet * [x] reactivate execution tests => will be done in a followup #2296 * [x] add specific tests => will be done in the followup #2296 Co-authored-by: Damir Vodenicarevic <damipator@gmail.com> Co-authored-by: damip <damipator@gmail.com>
18873b7
to
c37a5ee
Compare
c37a5ee
to
cf2e1a5
Compare
bors try |
tryBuild failed: |
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.
LGTM (nice improvements happen here)! With few comments 👍
Co-authored-by: Aurelia <56112063+AureliaDolo@users.noreply.github.com>
5e1183a
to
a9eaf83
Compare
Let's reboot this :) Prioritary review ! |
bors merge |
thanks @yvan-sraka that was fast ! |
Build succeeded: |
Add tests for the execution module & fix events.
Related with the issue #2079, we want to add test in the execution module. I suggest to not duplicate all tests, but test the layer of the massa usage. For example, we add the events and the current PR test if the implementation is good.
Add a unit test for the event generation, build a local wasm project on massa-execution build. Then execute the test.
Fixes: the events were not stored correctly in
final_events
Changes: Filter slot, include last slot.
this PR was a draft because we need to merge #2290 before