Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Move Truffle Test over to new decoder #2646

Merged
merged 17 commits into from
Dec 17, 2019
Merged

Move Truffle Test over to new decoder #2646

merged 17 commits into from
Dec 17, 2019

Conversation

haltman-at
Copy link
Contributor

@haltman-at haltman-at commented Nov 27, 2019

This PR moves Truffle Test over to using Truffle Decoder to decode events, rather than ethers (by means of web3). Now event decoding in Truffle Test is more comprehensive, more robust, and -- most obviously -- more colorful!

This PR has 3 commits. (Edit: Well, OK, it originally did anyway.) The first commit rewrites testrunner.js and soliditytest.js in async/await style, and adjusts tests.js to invoke them as such. Note that the two rewritten files aren't exported and so this shouldn't break anything.

The second resolves the merge conflict resulting from @CruzMolina replacing all uses of this.web3.utils with web3Utils.

(Then there's a third inbetween, oops.)

The last actually moves these two files over to using the new decoder. For soliditytest.js, this doesn't do very much other than move things over. For testrunner.js, this also means adding an indicator for anonymous events (which were previously omitted entirely), adding handling for ambiguous events, and using ResultInspector to nicely print the decoded values (with color!). I kept it all on a single line as it was before, regardless of length.

I did remove the type indicators that used to be printed; they seemed less necessary now. Let me know if you think those should be added back in.

Also, I didn't do anything to handle full mode vs ABI mode; I just printed decodings in whatever form they came in.

Anyway yeah that's basically it. Also I didn't change the compilation call over to the new compile interface, but I know that @gnidan is going to write a review telling me to do so. :P

@haltman-at
Copy link
Contributor Author

Hm, looks like this is failing CI, presumably because I didn't update tests; will have to see what's up wth that...

@haltman-at
Copy link
Contributor Author

Ugh, looks like I missed an await when moving things over to async/await. I'd rebase to make it part of that commit, but there's conflicts... (I am doing a force push so it'll at least be grouped with those)

@haltman-at
Copy link
Contributor Author

Hm -- looks like at least one of the test failures was actually due to a bug in the deployedBytecode getter/setter in contract. So, one of my commits now fixes that!

@haltman-at
Copy link
Contributor Author

And, another looks to have been due to a bug in codec...

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Minor formatting notes

packages/core/lib/testing/testrunner.js Outdated Show resolved Hide resolved
packages/core/lib/testing/testrunner.js Outdated Show resolved Hide resolved
depth: null,
colors: true,
maxArrayLength: null,
breakLength: Infinity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make breakLength be 80?

Suggested change
breakLength: Infinity,
breakLength: 80,

(or whatever, to get the lines to be a bit shorter... I guess this might require finagling to get the indent to work right)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I was just matching the old behavior which was printing the whole thing on one line. As you say, though, splitting over multiple lines will require some indent finagling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I did this, but in order for the result to make sense I also had to put each argument on a separate line. And so I also ended up putting a blank line between events like you suggested elsewhere. Also, I decided to reintroduce the type marker, because why not.

I am hoping the result is good? I encourage you to try it out and see what you think.

@coveralls
Copy link

coveralls commented Dec 13, 2019

Coverage Status

Coverage increased (+0.01%) to 72.387% when pulling 835ec60 on test-decoding into 54931ad on develop.

@haltman-at
Copy link
Contributor Author

Quick note: I've changed this so that it will now use definedIn rather than class when available -- like when you see Class.Event, it'll be the class that defined it. If that's not available it'll fall back to the class that emitted it, but this should only happen in cases where it shouldn't cause serious confusion.

@haltman-at
Copy link
Contributor Author

Minor note, I've stuck a commit in this PR to fix a problem in codec's typeString() function. It was supposed to only print the mutability if the mutability was something other than nonpayable; instead, I reversed the test and it only printed the mutability of the mutability was equal to nonpayable. Oops. Well, fixed now.

@haltman-at
Copy link
Contributor Author

Question: While it's not due to this PR, this is where it's coming up, so I'm wondering: Is the message you get when you try to decode an indexed argument of reference type (e.g. string) too long? This is basically the only place it appears at the moment, it could totally be shortened if you think it's necessary or think it can be improved.

@haltman-at
Copy link
Contributor Author

OK, after talking to @gnidan, I added some line-wrapping there. While I was at it I changed the existing line-wrapping logic to go based on total length rather than just the length of the first line...

@haltman-at
Copy link
Contributor Author

One last quick change before I merge, I've lowered breakLength to account for indentation somewhat.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants