test: Remove fragile assert_memory_usage_stable #17469

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1911-testFragileMem changing 2 files +15 −68
  1. MarcoFalke commented at 9:16 PM on November 13, 2019: member

    This test fails on arm64 and a fuzz tests seems inappropriate for the functional test suite anyway, so remove it.

    Example failures:

  2. MarcoFalke added the label Tests on Nov 13, 2019
  3. practicalswift commented at 10:49 AM on November 14, 2019: contributor

    ACK fadeb41fcce084c95fe29a9cb85a5df3c7df3cf1 -- diff looks good and Travis is happy

    Thanks for working on this

  4. laanwj commented at 3:51 PM on November 14, 2019: member

    ACK, maybe remove the definition of assert_memory_usage_stable in test_node.py as well (and get_mem_rss_kilobytes) as this is the only use.

  5. jamesob commented at 3:53 PM on November 14, 2019: member

    Concept ACK. RIP.

  6. MarcoFalke commented at 3:54 PM on November 14, 2019: member

    I wasn't sure if @jamesob was using it for some projects, but I interpret the reply as "no", so going to remove the assert_memory_usage_stable as well.

  7. jamesob commented at 3:55 PM on November 14, 2019: member

    FWIW I think this is a totally appropriate assertion for a functional test, since it's the highest-level suite of tests we have, and ensuring sane memory use is a feature of the system, but if we can't do it reliably across platform then we should either not do it on unreliable platforms or remove it entirely. My preference would be for the former, but if other people don't want to maintain this then I won't complain.

  8. test: Remove fragile assert_memory_usage_stable fac942ca57
  9. MarcoFalke force-pushed on Nov 14, 2019
  10. laanwj commented at 4:00 PM on November 14, 2019: member

    In theory I agree, in practice a modern OS is so complex with so much going on in the background, that deterministically testing memory use seems to be impossible. It's the same as for timing tests. Sure, this can be done in a controlled environment, but not Travis and definitely not with parallelism of many. (Edit: it'd probably work better with some LD_PRELOAD library that tracked malloc/free instead of parsing ps RSS output. But anyhow, if anyone wants to do that kinds of test in the future they can.)

  11. MarcoFalke commented at 4:03 PM on November 14, 2019: member

    I think the check itself is useful, but the limitations of the functional test framework make it almost useless in practice. The current status of this tests is: Running for twelve months, not a single reproducible failure.

    I think this can be implemented as a fuzz test, making it more powerful, useful and easier to debug in case of failure.

  12. jamesob approved
  14. MarcoFalke referenced this in commit c7709cbf4c on Nov 15, 2019
  15. MarcoFalke merged this on Nov 15, 2019
  16. MarcoFalke closed this on Nov 15, 2019
  17. MarcoFalke deleted the branch on Nov 15, 2019
  18. deadalnix referenced this in commit a382f87da6 on May 21, 2020
  19. ftrader referenced this in commit ab6e90f067 on Aug 17, 2020
  20. random-zebra referenced this in commit bffe509aed on Jun 28, 2021
  21. DrahtBot locked this on Dec 16, 2021
Contributors
MarcoFalkepracticalswiftlaanwjjamesob
Labels
Tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me