Skip to content

[Tests] Add mempool_limit.py test#7153

Merged
laanwj merged 2 commits into
bitcoin:masterfrom
jonasschnelli:2015/12/mempool-test
Dec 22, 2015
Merged

[Tests] Add mempool_limit.py test#7153
laanwj merged 2 commits into
bitcoin:masterfrom
jonasschnelli:2015/12/mempool-test

Conversation

@jonasschnelli
Copy link
Copy Markdown
Contributor

This will test mempools tx eviction (-maxmempool) together with the new conflict handling (#7105)

Comment thread qa/rpc-tests/mempool_limit.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you copied this code, DRY?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think this code part is in maxuploadtarget.py, prioritise_transaction.py and pruning.py. Someone should refactor that once. I have original taken this from a test that @sdaftuar wrote.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mind to do it in this PR like 826079e?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's enough to review this method once and not every time a pull tester PR is opened.

@jonasschnelli
Copy link
Copy Markdown
Contributor Author

Nits addresses: refactored the shared functions (moved to util.py).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you do the same here?

    def __init__(self):
        self.txouts = create_many_txouts()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This part needs to be refactored between 4 existing tests.
This PRs intent is not to refactor the test framework instead it should add a mempool limit/eviction test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider it a NIT but I don't like having the same code in 4 locations. You are guaranteed to get a bug out of that some day. (We already had it with other rpc tests)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Having the same code in 4 locations only makes sense if it is expected to diverge again later. Over-eager de-duplication can result in more complex code in such cases.

I don't think that's the case here. But I also agree with @jonasschnelli that refactoring is not required in a PR that adds a test.

@maflcko
Copy link
Copy Markdown
Member

maflcko commented Dec 3, 2015

utACK 7632cf6. Runs for 4 secs, so should be ok to run every time.

@laanwj laanwj merged commit 7632cf6 into bitcoin:master Dec 22, 2015
laanwj added a commit that referenced this pull request Dec 22, 2015
7632cf6 [Tests] Refactor some shared functions (Jonas Schnelli)
110ff11 [Tests] Add mempool_limit.py test (Jonas Schnelli)
@laanwj
Copy link
Copy Markdown
Member

laanwj commented Dec 22, 2015

utACK jonasschnelli@7632cf6

jonasschnelli added a commit that referenced this pull request Dec 22, 2015
- [Tests] Add mempool_limit.py test
- [Tests] Refactor some shared functions

Github-Pull: #7153
Rebased-From: 110ff11 7632cf6
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
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.

3 participants