test: Fix test by checking the actual exception instance #28989

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:231202-system-test changing 1 files +7 −1

hebasto commented at 9:57 PM on December 2, 2023: member

The system_tests/run_command test is broken because it passes even with the diff as follows:

--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -90,7 +90,7 @@ BOOST_AUTO_TEST_CASE(run_command)
         });
     }
     {
-        BOOST_REQUIRE_THROW(RunCommandParseJSON("echo \"{\""), std::runtime_error); // Unable to parse JSON
+        BOOST_REQUIRE_THROW(RunCommandParseJSON("invalid_command \"{\""), std::runtime_error); // Unable to parse JSON
     }
     // Test std::in, except for Windows
 #ifndef WIN32

The reason of such fragility is that the BOOST_REQUIRE_THROW macro passes even if the command raises an exception in the underlying subprocess implementation, which might have a type derived from std::runtime_error.

DrahtBot commented at 9:57 PM on December 2, 2023: contributor



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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK furszy, maflcko, pablomartin4btc, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot added the label Tests on Dec 2, 2023
hebasto removed the label Tests on Dec 2, 2023
DrahtBot added the label Tests on Dec 2, 2023

Type	Reviewers
ACK	furszy, maflcko, pablomartin4btc, achow101

in src/test/system_tests.cpp:103 in 24d978a36d outdated

  99 | +#else
 100 | +        const std::string command{"echo {"};
 101 | +#endif
 102 | +        BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const auto& e) {
 103 | +            return std::strstr(e.what(), "Unable to parse JSON: {") != nullptr;
 104 | +        });

furszy commented at 3:38 PM on December 3, 2023:

Could use HasReason().

BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON"));

hebasto commented at 4:08 PM on December 3, 2023:

Thanks! Updated.

test: Fix test by checking the actual exception instance

The BOOST_REQUIRE_THROW passes even if the command raises an exception
in the underlying subprocess implementation, which might have a type
derived from std::runtime_error.

55e3dc3e03

hebasto force-pushed on Dec 3, 2023
hebasto commented at 4:08 PM on December 3, 2023: member
Updated 24d978a36d1b405f968161a9d046beffffc0b249 -> 55e3dc3e03510e97caba1547a82e3e022b0bbd42 (pr28989.01 -> pr28989.02, diff):
- addressed @furszy's comment
furszy commented at 4:12 PM on December 3, 2023: member

Non-Windows code ACK 55e3dc3e
maflcko commented at 7:29 AM on December 4, 2023: member

lgtm ACK 55e3dc3e03510e97caba1547a82e3e022b0bbd42
pablomartin4btc commented at 12:05 AM on December 6, 2023: member

ACK 55e3dc3e03510e97caba1547a82e3e022b0bbd42
maflcko assigned achow101 on Dec 6, 2023
maflcko assigned ryanofsky on Dec 6, 2023
hebasto commented at 11:02 AM on December 6, 2023: member

Friendly ping @Sjors :)
achow101 commented at 3:27 PM on December 6, 2023: member

ACK 55e3dc3e03510e97caba1547a82e3e022b0bbd42
achow101 merged this on Dec 6, 2023
achow101 closed this on Dec 6, 2023
hebasto deleted the branch on Dec 6, 2023
Sjors commented at 6:39 PM on December 6, 2023: member

Looks like a reasonable fix, thanks!
PastaPastaPasta referenced this in commit d25cdb6000 on Oct 24, 2024
PastaPastaPasta referenced this in commit 285c53633b on Oct 24, 2024
PastaPastaPasta referenced this in commit 75e0334866 on Oct 24, 2024
PastaPastaPasta referenced this in commit a67319351a on Oct 24, 2024
bitcoin locked this on Dec 5, 2024

Contributors

Labels

test: Fix test by checking the actual exception instance #28989

Code Coverage

Reviews

Conflicts