Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Sep 2, 2021

For some tests the reject reason wasn't cleared between runs and thus subsequent tests might (theoretically) fail to verify the correct reject reason.

@fanquake fanquake added the Tests label Sep 2, 2021
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

Approach Ack fa1b08e
Introducing 2 separate functions to handle the manual logic is a great way to go ahead and follow the DRY principle in our codebase!

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

ACK fa1b08e

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK fa1b08e

Nice improvement 🎉

@maflcko maflcko merged commit 5fb6701 into bitcoin:master Sep 3, 2021
@maflcko maflcko deleted the 2109-testRej branch September 3, 2021 15:50
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 4, 2021
…x test

fa1b08e test: Always clear reject reason in IsStandard tx test (MarcoFalke)

Pull request description:

  For some tests the reject reason wasn't cleared between runs and thus subsequent tests might (theoretically) fail to verify the correct reject reason.

ACKs for top commit:
  benthecarman:
    ACK fa1b08e
  theStack:
    Code-review ACK fa1b08e

Tree-SHA512: fcb727a690f92a4cf06127c302ba464f1e8cb997498e4f7fd9e210d193559b07e6efdb9d5c8a0bef3fe643bdfd5fedd431aaace20978dd49e56b8e770cb9f930
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants