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

Fixes false positives in expectTransactionFailedAsync #1852

Merged

Conversation

hysz
Copy link
Contributor

@hysz hysz commented Jun 5, 2019

Description

The function expectTransactionFailedAsync(transactionReceipt, expectedRevertReason) returns a false positive when the expected revert reason is a substring of the exception message.

For example, if the expected revert reason is INVALID_DATA and the actual reason is INVALID_DATA_OFFSET then there will be a false positive.

To go one step further, the full exception message is VM Exception while processing transaction: revert INVALID_DATA_OFFSET. Any substring within that message will also return a false positive, for example, processing trans.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@hysz hysz requested a review from abandeali1 as a code owner June 5, 2019 01:04
@coveralls
Copy link

coveralls commented Jun 5, 2019

Coverage Status

Coverage decreased (-0.02%) to 83.353% when pulling 56bc294 on fix/contracts/expectTransactionFailedAsyncFalsePositives into b2cf701 on development.

@hysz hysz force-pushed the fix/contracts/expectTransactionFailedAsyncFalsePositives branch from dcd86f1 to 56bc294 Compare June 5, 2019 01:53
@hysz hysz changed the title [WIP] Fixes false positives in expectTransactionFailedAsync Fixes false positives in expectTransactionFailedAsync Jun 5, 2019
@@ -160,7 +161,8 @@ export async function expectTransactionFailedWithoutReasonAsync(p: sendTransacti
* otherwise resolve with no value.
*/
export async function expectContractCallFailedAsync<T>(p: Promise<T>, reason: RevertReason): Promise<void> {
return expect(p).to.be.rejectedWith(reason);
const rejectionMessageRegex = new RegExp(`^VM Exception while processing transaction: revert ${reason}$`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will work in Geth. We have disabled Geth tests for now (see #1754 and #1775). Going forward, instead of running all of our unit tests through Geth and relying on features that don't exist upstream, we want to have integration tests that operate at a higher level and don't rely on any extra features.

Anyways, I'm okay with this for now but if we write integration tests for Geth in the future we'll have to update it.

@albrow
Copy link
Contributor

albrow commented Jun 5, 2019

To go one step further, the full exception message is VM Exception while processing transaction: revert INVALID_DATA_OFFSET. Any substring within that message will also return a false positive, for example, processing trans.

Oof. If this is true we should report the issue to Mocha/Chai. I can't imagine this was the intended behavior.

@albrow albrow self-requested a review June 5, 2019 20:22
Copy link
Contributor

@albrow albrow left a comment

Choose a reason for hiding this comment

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

Like I mentioned we might have to change this in the future to support revert reasons from Geth (and maybe Parity too), but I'm okay with leaving it as is for now.

@hysz hysz merged commit 8453c61 into development Jun 5, 2019
@hysz
Copy link
Contributor Author

hysz commented Jun 5, 2019

Oof. If this is true we should report the issue to Mocha/Chai. I can't imagine this was the intended behavior.

@albrow I had the same thought and checked out their source code (here). Funny enough it does appear to be intentional ¯\_(ツ)_/¯...

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

Successfully merging this pull request may close these issues.

4 participants