Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_node: get_mem_rss fixups #14693

Merged
merged 1 commit into from Nov 12, 2018

Conversation

Projects
None yet
5 participants
@MarcoFalke
Copy link
Member

commented Nov 8, 2018

Follow up to #14522:

  • Fix math (Memory usage increase relative to previous memory usage, not final memory usage)
  • remove shell=True
  • assert that the node is running
  • Make it work on BSD-like systems

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1811-qaTestNodeFixups branch 2 times, most recently Nov 8, 2018

@jamesob

jamesob approved these changes Nov 8, 2018

Copy link
Member

left a comment

ACK

Thanks for the fixes!

test/functional/test_framework/test_node.py Outdated
#
# We could later use something like `psutils` to work across platforms.
except Exception:
except (subprocess.SubprocessError, ValueError):

This comment has been minimized.

Copy link
@jamesob

jamesob Nov 8, 2018

Member

We could also try detecting/parsing the *BSD-style output, which looks like "RSS\n 123".

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1811-qaTestNodeFixups branch 3 times, most recently Nov 8, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1811-qaTestNodeFixups branch to fa9ed38 Nov 8, 2018

@lucash-dev

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

utACK

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

utACK fa9ed38

@laanwj laanwj merged commit fa9ed38 into bitcoin:master Nov 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 12, 2018

Merge #14693: test_node: get_mem_rss fixups
fa9ed38 test_node: get_mem_rss fixups (MarcoFalke)

Pull request description:

  Follow up to #14522:

  * Fix math (Memory usage increase relative to previous memory usage, not final memory usage)
  * remove `shell=True`
  * assert that the node is running
  * Make it work on BSD-like systems

Tree-SHA512: fc1b4f88173914b6cb6373655cffd781044a0c146339e3fa90da03b197faa20954567a77335965b857d29d27f32661698b6a0340f0c616f643b8c4510cd360c2

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1811-qaTestNodeFixups branch Nov 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.