Upgrade wagon and add tests #93
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Silas Davis <[email protected]>
silasdavis
force-pushed
the
upgrade-wagon
branch
2 times, most recently
from
ddb6181
to
562dcda
Compare
Signed-off-by: Silas Davis <[email protected]>
silasdavis
force-pushed
the
upgrade-wagon
branch
from
562dcda
to
0b74cb1
Compare
Signed-off-by: Silas Davis <[email protected]>
silasdavis
commented
Oct 9, 2019
| @@ -442,12 +442,12 @@ func (c *SSAFunctionCompiler) Compile(importTypeIDs []int) { | |||
| c.PushStack(targetValueID) | |||
| } | |||
|
|
|||
| case "current_memory": | |||
| case "memory.size": | |||
There was a problem hiding this comment.
We could avoid the strings in this switch if upstream merges: go-interpreter/wagon#172
There was a problem hiding this comment.
Agreed; I left a suggestion comment over in go-interpreter/wagon#172. It's not absolutely mandatory, though regardless of whichever option is chosen for converting an opcode into a string, we should definitely incorporate the change int a separate PR.
|
Thanks a lot for the PR! Merged in #89 just recently; would love to start incorporating tests into the CI cycle. Let me know when you're able to rebase and then LGTM. Happy with the Makefile approach, which is what we take for other projects we have in the organization. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
edit: oops I missed this: https://github.com/perlin-network/life/pull/89/files. I think the tests still have value so I can rebase once #89 is merged if you want to accept these changes
This helps alleviate a lot of downstream pain with replace directives needed for transitive wagon dep.
I also found it slightly painful to run tests so I have added a makefile and also a github actions config. To enable this you would need to join the github actions beta here: https://github.com/features/actions
Makefiles are a little arcane, but they are quite standard in Go and I feel like a better tool for the job than the current python script. Let me know if you think that script could be removed or if you want me to tweak the makefile approach.
I've added the test suite as a git submodule so
make testwill now generate wast and run tests provided you have wast2json on your path.