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: Avoid connecting to real network when running tests #21254

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Feb 21, 2021

Introduced in #19884

@fanquake fanquake added the Tests label Feb 21, 2021
MarcoFalke added 2 commits February 21, 2021 11:01
Every (sub)test in the framework assumes the node is running, except for
the (sub)tests in this file. Remove that confusion by stopping the node
at the start of every subtest, instead of at the end.
Can be reviewed with --word-diff-regex=.
@maflcko maflcko force-pushed the 2102-testNoTelemetry branch from fa2240e to fa730e9 Compare February 21, 2021 10:03
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Sjors
Copy link
Member

Sjors commented Feb 23, 2021

ACK fa730e9

Can you add a note which test was connecting to the outside world? I assume it's one of the few tests that uses mainnet?

@maflcko
Copy link
Member Author

maflcko commented Feb 23, 2021

I haven't checked if any of the current tests reach out right now, but any non-regtest test might be affected.

@practicalswift
Copy link
Contributor

Concept ACK

Good catch! Accidental telemetry is still telemetry and should be removed.

@maflcko maflcko merged commit c0e44ee into bitcoin:master Feb 25, 2021
@maflcko maflcko deleted the 2102-testNoTelemetry branch February 25, 2021 13:46
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2021
…ing tests

fa730e9 test: Avoid connecting to real network when running tests (MarcoFalke)
fa1b713 test: Assume node is running in subtests (MarcoFalke)

Pull request description:

  Introduced in bitcoin#19884

ACKs for top commit:
  Sjors:
    ACK fa730e9

Tree-SHA512: fe132a9ffe2fae1ab16857a3dec9839526fdf74d27a1ae794fbffca8356f639c4b916dc888b260281e9cc793916706c18d1687ebb5a076d4e1c481d218d308d3
@practicalswift
Copy link
Contributor

To detect accidental introduction of telemetry in the future: could we perhaps guard against external connections in tests via say a seccomp restricted job in Travis? Perhaps nsjail or firejail could be used.

@maflcko
Copy link
Member Author

maflcko commented Feb 25, 2021

Sure, pull request welcome

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
Can be reviewed with --word-diff-regex=.

Github-Pull: bitcoin#21254
Rebased-From: fa730e9
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 27, 2022
Summary:
Backport of [[bitcoin/bitcoin#21254 | core#21254]].

This is the complete fix for the p2p_dos_header_tree.py test, so the temporary fix introduced in D10908 can be reverted.

Depends on D10909.

Test Plan:
  ninja all check-functional

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D10911
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 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