Skip to content

Conversation

gwillen
Copy link
Contributor

@gwillen gwillen commented Oct 29, 2020

Add a configure flag to disable running secp tests with every make check.

Hopefully this isn't too controversial -- I recognize the importance of testing libsecp, and obviously those tests will remain enabled by default. But this flag is useful when doing automated repeated runs of the bitcoin core test suite, to avoid repeatedly re-running the (somewhat expensive) libsecp test suite when libsecp is not changing (since it only gets re-vendored occasionally.)

Add a configure flag to disable running secp tests with every `make check`.
@luke-jr
Copy link
Member

luke-jr commented Oct 29, 2020

Why not just run the tests you want directly, instead of the all-tests make check wrapper?

@gwillen
Copy link
Contributor Author

gwillen commented Oct 29, 2020

Hmm, that's a good question. Mostly I don't quite know how to do that. I see that make check runs the following things:

make -C src/ check-TESTS (which I definitely want)
test/util/bitcoin-util-test.py (which I maybe don't actually want, but it only takes a couple seconds)
test/util/rpcauth-test.py (which I don't actually know what it tests, but it runs instantly)
bench/bench_bitcoin (which I probably want, I think)
make -C src/secp256k1/ check (which I don't want)
make -C src/univalue/ check (which is probably irrelevant but also runs instantly)

So it might be that make -C src/ check-TESTS would actually do what I want. Is the above actually a comprehensive list of what make check runs?

@sipa
Copy link
Member

sipa commented Oct 29, 2020

I think the stuff currently covered by make check is kind of arbitrary anyway. Why doesn't it include the functional tests, for example?

@gwillen
Copy link
Contributor Author

gwillen commented Oct 29, 2020

Yeah, I had previously (before today) mostly thought of make check as just running the stuff that's actually make -C src/ check-TESTS. I didn't know about some of the other stuff.

I think it makes sense for make check to mostly run stuff that's fast and/or really important, and the functional tests to be slower and more thorough. I assume "really important" is the argument for why the secp tests run by default, even though they are a bit slow (although similar to the main unit tests in speed I guess), and unlikely to break.

@gwillen
Copy link
Contributor Author

gwillen commented Oct 29, 2020

Ok for what it's worth, I'm convinced by @luke-jr 's point that I would be better off having my script just directly run the parts of make check that I want. Can someone confirm or refute my list of them above? After that I don't strongly care about this PR anymore, except insofar as it produces useful discussions of how the tests ought to be organized.

@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.

@fanquake
Copy link
Member

~0. I don't think there's a need for us to add a configure option here. If you're trying to save resources I'd also suggest just disabling targets like the benchmarks (--disable-bench) if you aren't interested in compiling / running them. You can also run tests directly using src/test/test_bitcoin --run_test=scriptpubkeyman_tests,init_tests,some_more_tests etc.

@gwillen
Copy link
Contributor Author

gwillen commented Oct 30, 2020

I'm going to close this just to save on people's attention, since I have a better way and it doesn't seem like anybody's particularly in love with it. If someone is in fact in love with it, I'm happy to reopen.

@gwillen gwillen closed this Oct 30, 2020
kristapsk added a commit to JoinMarket-Org/joinmarket-clientserver that referenced this pull request Nov 6, 2020
aa3cec4 Make libsecp256k1 tests optional (Kristaps Kaupe)

Pull request description:

  They are expensive and slow, especially on a slow hardware like
  Raspberry Pi.

  Inspired by bitcoin/bitcoin#20264.

Top commit has no ACKs.

Tree-SHA512: ad23f8bab1f95fb5d0d7c6f2bcbe1a6950f7f62dd82e8d34d65abf6dffcfd1e7f9100d7c1423d1d8764ba6ce5145f5df17035130314432bb6e44361e7d773960
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants