Determine which unit tests are impacted by the changes and run them.
The arc unit command starts by running a build, then it uses the
dependency tracking tool from Ninja to determine which test is impacted
by the changes.
Depends on D2792
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project
Initial state: the previous commit does not modify a c, h or cpp file.
arc unit
Should return:
SKIP No unit test to run arc unit --trace --everything
Should return:
PASS <duration> Bitcoin unit tests
Check in the trace output that all the unit tests are run.
Add a comment in the file `src/test/addrman_tests.cpp'
arc unit --trace
Should return:
PASS <duration> Bitcoin unit tests
Check in the trace output that the addrman_tests test is run.
Reset the changes in `src/test/addrman_tests.cpp'.
Add a comment in the file `src/addrman.cpp'
arc unit --trace
Should return:
PASS <duration> Bitcoin unit tests
Check in the trace output that the following tests are run:
addrman_tests,avalanche_tests,DoS_tests,main_tests,net_tests
Reset the changes in `src/addrman.cpp'.
Add a comment in the file `src/addrman.h'
arc unit --trace
Should return:
PASS <duration> Bitcoin unit tests
Check in the trace output that the same tests are run than in the
previous list.
Reset the changes in `src/addrman.h'.
Add a comment in the file `src/wallet/wallet.h'
arc unit --trace
Should return:
PASS <duration> Bitcoin unit tests
Check in the trace output that the following tests are run:
wallet_test_fixture,accounting_tests,wallet_tests,walletdb_tests
Reset the changes in `src/wallet/wallet.h'.
In the file src/test/addrman_tests.cpp, modify the line (currently
87):
BOOST_CHECK_EQUAL(addrman.size(), 0);
To:
BOOST_CHECK_EQUAL(addrman.size(), 1);
Then run:
arc unit
Should return:
FAIL addrman_tests.cpp addrman_tests/addrman_simple(87): addrman.size() == 1
Still in the file src/test/addrman_tests.cpp, modify the line
(currently 94):
BOOST_CHECK_EQUAL(addrman.size(), 1);
To:
BOOST_CHECK_EQUAL(addrman.size(), 2);
In the file src/test/allocator_tests.cpp, modify the line (currently
24):
BOOST_CHECK(chunk != nullptr);
To:
BOOST_CHECK(chunk == nullptr);
Then run:
arc unit
Should return:
FAIL addrman_tests.cpp addrman_tests/addrman_simple(87): addrman.size() == 1 addrman_tests/addrman_simple(94): addrman.size() == 2 FAIL allocator_tests.cpp allocator_tests/arena_tests(24): chunk == nullptr
Reset the changes in src/test/addrman_tests.cpp and
src/test/allocator_tests.cpp.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcanist_unit
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 5436 Build 8934: Bitcoin ABC Buildbot (legacy) Build 8933: arc lint + arc unit
Event Timeline
Comment Actions
| arcanist/unit/ABCUnitTestEngine.php | ||
|---|---|---|
| 61 ↗ | (On Diff #7318) | This could be an array of valid test paths, which can be provided at the top of the file for better discovery / grep-ability |
Comment Actions
I made numerous comments on the implementation as it's probably useful feedback, the code itself being fragile and quite hacky, but I do not think this is fixable as the approach makes no sense to begin with. This just run a random test binary in a way that cannot be relied upon in any workflow using heuristics that hardly catch anything.
| arcanist/unit/ABCTestResultParser.php | ||
|---|---|---|
| 11 ↗ | (On Diff #7419) | list( $fullmaskwhateverthatmeans, $name, $line, $testCase, $failedCheckWhateverThatIs, ) = $error; And not only do you not need a comment, but the rest of the code also magically becomes readable without a comment. It is also very clear that $error is maybe not an error as its name seems to imply. |
| 17 ↗ | (On Diff #7419) | Just use an array. You can always implode it if you want to print it one item per line at point of use. |
| 35 ↗ | (On Diff #7419) | Don't pass strings as lambda. |
| 49 ↗ | (On Diff #7419) | remove |
| arcanist/unit/ABCUnitTestEngine.php | ||
| 10 ↗ | (On Diff #7419) | trailing coma |
| 25 ↗ | (On Diff #7419) | That seems like the best way to chose what build options to test under. |
| 47 ↗ | (On Diff #7419) | This will just run some random build. |
| 53 ↗ | (On Diff #7419) | Maybe return !is_null($this->testBinary) ? |
| 75 ↗ | (On Diff #7419) | Using the tree structure rather than filenames seems like a more apt approach. If you change one of the header in the wallet code, it is likely that any wallet test is relevant, not just the one that happen to have the same name as the header file. This means that most test will match. Yes, it will. t is because the codebase is a monolithic blob and has nothing to do with the heuristic used here. Creating a flacky heuristic because the structure of the project sucks hardly seems like it's moving things is the right direction. |
| 80 ↗ | (On Diff #7419) | I'm pretty sure you are not looking to attach your name to a CVE, so I would advice using proper techniques to build command lines out of variables. |
| 85 ↗ | (On Diff #7419) | I'm pretty sure the goal of creating future here is to have some level of parallelism. |
| 147 ↗ | (On Diff #7419) | This flow is very strange. It generates the command, then pass the command down to another function. Both only have one callsite and seems to rely on each other doing some work in way the other expect. It also brakes the parallelism afforded by the use of a future to boot. |
| 158 ↗ | (On Diff #7419) | The fact both git and phabricator complain about this is kind of an hint. If you add something later in the file, it'll count as a modification of the last existing line, and can even create a merge conflict. |
| .arcunit | ||
|---|---|---|
| 7 ↗ | (On Diff #7419) | Why ? |
Comment Actions
Complete rework.
This new proposal uses Ninja to find the dependencies and run the tests.
It is build upon the arc build command to ensure reproductible behavior.
| arcanist/configuration/ArcanistBitcoinABCConfiguration.php | ||
|---|---|---|
| 7 ↗ | (On Diff #8036) | Here I choose to build in the default directory, without clearing the CMake cache. |
| arcanist/configuration/ArcanistBitcoinABCConfiguration.php | ||
|---|---|---|
| 7 ↗ | (On Diff #8036) | Since the last arc build update, the directory (and the cache as a consequence) is always deleted after the build. |