Skip to content

Conversation

@jakub-wojciechowski
Copy link
Contributor

After spotting a problem with checking if a contract correctly throws an error in tests as explained in #323 , I've decided to find similar cases in the codebase and fix them all.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 83.508% when pulling 5e78475 on jakub-wojciechowski:master into 4d91118 on OpenZeppelin:master.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Great idea! Thanks!

Using return for control flow in this way seems like it could cause some headaches in the future. Could you change these tests to use the following equivalent pattern?

try {
  await doSomething();
  assert.fail('should have thrown');
} catch (e) {
  assertJump(e);
  /* extra validations here */
}

@jakub-wojciechowski
Copy link
Contributor Author

Thx @frangio for the suggestion. I agree it seems to be a better design. I've refactored other occurrences of returning in catch block to be consistent.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 83.421% when pulling b3f60b9 on jakub-wojciechowski:master into 4d91118 on OpenZeppelin:master.

@frangio frangio merged commit 60bc6a6 into OpenZeppelin:master Jul 23, 2017
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants