Skip to content

test: Run AppInitSanityChecks before all tests #21763

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.

Sign up for GitHub

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

Jump to bottom
Closed
maflcko wants to merge 2 commits into from
Closed

test: Run AppInitSanityChecks before all tests #21763

maflcko wants to merge 2 commits into from

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Apr 23, 2021 •
edited
Loading

Instead of running the sanity checks as part of a single test case, run them as part of the normal setup for each test case. Benefits are:

  • Errors during test development (e.g. wrong datadir) are caught early in the setup
  • It makes the test setup procedure closer to the bitcoind setup

@fanquake fanquake added the Tests label Apr 23, 2021
@maflcko maflcko force-pushed the 2104-testCommonInit branch from fa81e13 to fa8145a Compare April 23, 2021 09:23
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 24, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

This was referenced Apr 25, 2021
Merged
Closed
brunoerg
brunoerg approved these changes May 8, 2021
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tACK fa8145af797a6dac2cff049399fc8d85fd30fe0b

Ran the tests several times and seems ok (MacOS 11.3)

@fanquake
Copy link
Member

fanquake commented May 12, 2021

Concept ACK

@maflcko
Copy link
Member Author

maflcko commented May 12, 2021

@ryanofsky Any Concept NACK thoughts on this?

ryanofsky
ryanofsky approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

@ryanofsky Any Concept NACK thoughts on this?

Code review ACK fa8145af797a6dac2cff049399fc8d85fd30fe0b, and this seems fine to me. I do have an irrational dislike of the sanity checks because they throw exceptions and cause GDB catch throw false positives. Also in the future, I think it would be good to strip down the base test class and make extra setup like this optional. But this seems like a good change to make for current tests to be more consistent with the app.

@laanwj
Copy link
Member

laanwj commented May 14, 2021

It seems somewhat overkill to run the sanity tests for every test, but they run fast so also no concept NACK.

@DrahtBot DrahtBot mentioned this pull request Sep 15, 2021
Closed
@DrahtBot DrahtBot mentioned this pull request Oct 14, 2021
Merged
@DrahtBot DrahtBot mentioned this pull request Oct 27, 2021
Merged
@DrahtBot DrahtBot mentioned this pull request Nov 13, 2021
Merged
@pg156
Copy link

pg156 commented Dec 19, 2021

Reviewed fa8145a. Unit and functional tests are run successfully.

Errors during test development (e.g. wrong datadir) are caught early in the setup

I can see LockDataDirectory is called inside AppInitSanityChecks. But how do I manually generate an error by providing the wrong datadir, for both before and after this change, to verify the error is caught earlier after the change? E.g. can I give a wrong datadir to the command test_bitcoin?

After this change, 3 sanity checks (stdlib, secp256k1, chrono) which were in sanity_tests.cpp are now called in AppInitSanityChecks in setup_common.cpp, for all test cases. As the same AppInitSanityChecks is called in bitcoind.cpp, I agree individual tests setup are closer to the bitcoind setup, which is generally desirable. But I feel I don't have enough experience and knowledge to consider all pluses and minuses to give an ACK.

@maflcko maflcko force-pushed the 2104-testCommonInit branch 2 times, most recently from fae4252 to 2222701 Compare March 21, 2022 16:29
@maflcko
Copy link
Member Author

maflcko commented Mar 21, 2022

they throw exceptions and cause GDB catch throw false positives.

I think any code may throw exceptions, so I am wondering if this is something specific to init code?

Also in the future, I think it would be good to strip down the base test class and make extra setup like this optional.

Agree, though the ECCVerifyHandle is pretty basic. If you don't need that, you likely don't need any setup at all.

@maflcko
Copy link
Member Author

maflcko commented Mar 21, 2022

I can see LockDataDirectory is called inside AppInitSanityChecks. But how do I manually generate an error by providing the wrong datadir, for both before and after this change, to verify the error is caught earlier after the change? E.g. can I give a wrong datadir to the command test_bitcoin?

Without any warranty, this may be possible by removing g_insecure_rand_ctx_temp_path.rand256().ToString() or replacing it with a constant literal string, so that m_path_root will conflict with other's when multiple tests are run in parallel (make check -j 2 or larger).

This was referenced May 26, 2022
Merged
Merged
@maflcko maflcko force-pushed the 2104-testCommonInit branch from 2222701 to fa532cd Compare May 30, 2022 09:38
test: Run SetupEnvironment first
fa2c04a 
This is also run first in the main() function of bitcoind
@maflcko maflcko force-pushed the 2104-testCommonInit branch from fa532cd to fa7e110 Compare July 22, 2022 12:56
@maflcko maflcko force-pushed the 2104-testCommonInit branch from fa7e110 to fa607f8 Compare July 22, 2022 13:03
jamesob
jamesob approved these changes Jul 22, 2022
View reviewed changes
Copy link
Contributor

@jamesob jamesob left a comment

Choose a reason for hiding this comment

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

Github ACK fa607f8

More closely resembles bitcoind startup and removes code.

@maflcko maflcko requested a review from ryanofsky July 22, 2022 15:05
test: Run AppInitSanityChecks before all tests
facdfb6 
Also remove sanity unit tests, as they are run before all tests.
@maflcko maflcko force-pushed the 2104-testCommonInit branch from fa607f8 to facdfb6 Compare July 22, 2022 15:36
@maflcko
Copy link
Member Author

maflcko commented Jul 22, 2022

Force pushed to remove one more unit test

ryanofsky
ryanofsky approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK facdfb6

src/test/sanity_tests.cpp

#include <boost/test/unit_test.hpp>

BOOST_FIXTURE_TEST_SUITE(sanity_tests, BasicTestingSetup)
Copy link
Contributor

@ryanofsky ryanofsky Aug 2, 2022

Choose a reason for hiding this comment

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

In commit "test: Run AppInitSanityChecks before all tests" (facdfb6)

I wish this commit would just add the AppInitSanityChecks assert without deleting the other tests. I don't see a reason to delete low level tests just because high level code calls some of the same functions. Low level tests can be useful because they can find identify errors that might be masked in high level tests that are changing other state. Also they can be provide more granular errors in case of failure, and they can serve as examples to show how low-level APIs can be called.

@maflcko
Copy link
Member Author

maflcko commented Aug 3, 2022

Closing for now to redirect review to other open pulls.

@maflcko maflcko closed this Aug 3, 2022
@maflcko maflcko deleted the 2104-testCommonInit branch August 3, 2022 08:54
@bitcoin bitcoin locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@jamesob jamesob jamesob approved these changes

@brunoerg brunoerg brunoerg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @DrahtBot @fanquake @laanwj @pg156 @jamesob @ryanofsky @brunoerg