Skip to content

Conversation

yancyribbens
Copy link
Contributor

Perhaps this goes without saying, however the unit tests require make install in addition to make for changes to be reflected after modification.

@sipa
Copy link
Member

sipa commented Feb 9, 2020

This is wrong, you don't need to install the build before you can test it.

@yancyribbens
Copy link
Contributor Author

yancyribbens commented Feb 9, 2020

@sipa thanks for the feedback. Perhaps there is something with boost that i'm not familiar with then. If I update the test:

diff --git a/src/test/getarg_tests.cpp b/src/test/getarg_tests.cpp
index 10fb05ca8..5e0f48855 100644
--- a/src/test/getarg_tests.cpp
+++ b/src/test/getarg_tests.cpp
@@ -146,15 +146,7 @@ BOOST_AUTO_TEST_CASE(intarg)
 
 BOOST_AUTO_TEST_CASE(doubledash)
 {
-    const auto foo = std::make_pair("-foo", ArgsManager::ALLOW_ANY);
-    const auto bar = std::make_pair("-bar", ArgsManager::ALLOW_ANY);
-    SetupArgs({foo, bar});
-    ResetArgs("--foo");
-    BOOST_CHECK_EQUAL(gArgs.GetBoolArg("-foo", false), true);
-
-    ResetArgs("--foo=verbose --bar=1");
-    BOOST_CHECK_EQUAL(gArgs.GetArg("-foo", ""), "verbose");
-    BOOST_CHECK_EQUAL(gArgs.GetArg("-bar", 0), 1);
+    BOOST_TEST_MESSAGE("foo");
 }

and then run make, followed by test_bitcoin --log_level=message --run_test=getarg_tests/doubledash I see no message. however, after running make install, the unit test prints the message foo.

@theStack
Copy link
Contributor

theStack commented Feb 9, 2020

@yancyribbens: It seems that your problem is caused by omitting any path name on executing test_bitcoin, which will search in $PATH for the binary, which then most likely is the installed one. To execute unit tests from your local build, either use make check or src/test/test_bitcoin (as described in src/test/README.md).

@yancyribbens yancyribbens force-pushed the update-unit-test-readme branch from 23f90dd to 8729fb9 Compare February 10, 2020 08:49
@yancyribbens
Copy link
Contributor Author

@theStack thanks that works. Maybe it would be helpful to add the local path prefix under the section Running individual tests since running the system installed binary isn't useful here.

@fanquake fanquake changed the title doc: update unit test documentation to be more descriptive about how to mo… doc: add src/test/ prefix to test_bitcoin in test README Feb 20, 2020
@fanquake
Copy link
Member

Adding the prefix here seems fine, as we use it throughout the rest of the file. You just need to fix the commit message. i.e doc: add src/test/ prefix to test_bitcoin in test README.

@laanwj
Copy link
Member

laanwj commented Feb 28, 2020

Yes, I think this is fine, I mean it depends on what the current directory is, whether you want to do ./test_bitcoin or src/test/test_bitcoin, I've used both many times.

@yancyribbens
Copy link
Contributor Author

since src/test/test_bitcoin is used elsewhere it is helpful to be consistent imo. Also, I think this illustrates the binary test_bitcoin should not be the one installed with make install.

@laanwj
Copy link
Member

laanwj commented Mar 2, 2020

I think this illustrates the binary test_bitcoin should not be the one installed with make install.

This is intentionally done because the test_bitcoin is distributed as part of the downloads. People are encouraged to run test_bitcoin on any system they intend to use bitcoind on to increase confidence that there aren't any strange platform-specfic issues interfering.

@yancyribbens
Copy link
Contributor Author

yancyribbens commented Mar 2, 2020

This is intentionally done because the test_bitcoin is distributed as part of the downloads. People are encouraged to run test_bitcoin on any system they intend to use bitcoind on to increase confidence that there aren't any strange platform-specfic issues interfering.

That's understandable. However, the instuctions ask you to run make after modifying a test file. Running make only updated the file at src/test/test_bitcoin and not the system installed bin. Otherwise the instructions should include make install in addition to make imo.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 1, 2020

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

Conflicts

No conflicts as of last run.

@yancyribbens yancyribbens force-pushed the update-unit-test-readme branch from 287b0a8 to c3e369d Compare April 1, 2020 11:06
@maflcko
Copy link
Member

maflcko commented Apr 28, 2020

This simple change seems too controversial and didn't attract too much attention in the past month. Closing for now.

@maflcko maflcko closed this Apr 28, 2020
@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.

7 participants