Add check that gdb is on the path #4420

xbauch · 2019-03-22T14:38:23Z

No functional change only wrapped the test-cases in if-statements checking
that gdb can be called. The rest is clang-formatting.

Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

smowton · 2019-03-22T14:43:24Z

unit/memory-analyzer/gdb_api.cpp

@@ -104,94 +104,111 @@ void gdb_api_internals_test()
 TEST_CASE("gdb api internals test", "[core][memory-analyzer]")
 {
 #ifdef RUN_GDB_API_TESTS
-  gdb_api_internals_test();
+  int has_gdb = system("gdb --version");


How about bool has_gdb = (system("gdb --version") == 0);?

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 65a30ea).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105460761
Status will be re-evaluated on next push.
Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).
the compatibility was already broken by an earlier merge.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: b3c6c40).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105462720

peterschrammel · 2019-03-22T22:49:00Z

unit/memory-analyzer/gdb_api.cpp

@@ -104,94 +104,111 @@ void gdb_api_internals_test()
 TEST_CASE("gdb api internals test", "[core][memory-analyzer]")
 {
 #ifdef RUN_GDB_API_TESTS
-  gdb_api_internals_test();
+  bool has_gdb = system("gdb --version") == 0;


peterschrammel · 2019-03-22T22:49:53Z

unit/memory-analyzer/gdb_api.cpp

      }
+      catch(const gdb_interaction_exceptiont &e)
+      {
+        std::cerr


Don't we use message handlers in unit tests?

At least not consistently, I found a few other tests that call std:cerr. Should I modify this one?

@peterschrammel What value would the use of message handlers add here?

peterschrammel · 2019-03-22T22:50:13Z

unit/memory-analyzer/gdb_api.cpp

 #endif
 }

 TEST_CASE("gdb api test", "[core][memory-analyzer]")
 {
 #ifdef RUN_GDB_API_TESTS
-  compile_test_file();
-
+  bool has_gdb = system("gdb --version") == 0;


tautschnig · 2019-03-23T08:32:18Z

unit/memory-analyzer/gdb_api.cpp

@@ -104,94 +104,111 @@ void gdb_api_internals_test()
 TEST_CASE("gdb api internals test", "[core][memory-analyzer]")
 {
 #ifdef RUN_GDB_API_TESTS
-  gdb_api_internals_test();
+  const bool has_gdb = system("gdb --version") == 0;


I would prefer if run("gdb", {"gdb", "--version"}) (from util/run.{h,cpp}) were used in place of system (system is really heavy-weight as it starts a shell). But I do acknowledge that we already use system in this file, though I think that call should equally be replaced.

Replaced. The other use was replaced in the next commit.

tautschnig · 2019-03-23T08:33:34Z

unit/memory-analyzer/gdb_api.cpp

 #endif
 }

 TEST_CASE("gdb api test", "[core][memory-analyzer]")
 {
 #ifdef RUN_GDB_API_TESTS
-  compile_test_file();
-
+  const bool has_gdb = system("gdb --version") == 0;


As above - or maybe even factor out this check (and the SECTION) into a separate procedure.

tautschnig · 2019-03-23T08:35:03Z

unit/memory-analyzer/gdb_api.cpp

      }
+      catch(const gdb_interaction_exceptiont &e)
+      {
+        std::cerr


@peterschrammel What value would the use of message handlers add here?

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 2af9fff).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105533191

martin-cs · 2019-03-23T11:38:23Z

OK, so today I'm going to be that bell-end that uses a simple patch to complain about something only semi-related. Given that : why only gdb? I believe there are various other tools that implement compatable text annd / or protocol interfaces. Explicitly testing for gdb seems to tie us to that.

( Comment not review so you can (and I will not be even the slightest bit offended) ignore this. )

xbauch · 2019-03-23T20:44:41Z

@martin-cs The context is that this particular unit-test hangs indefinitely if the system does not have gdb on the PATH. I guess if there are other tools with similar interface they don't cause this problem (or at least not when running unit).

tautschnig · 2019-03-23T21:24:33Z

@xbauch clang-format asks for reformatting two lines.

martin-cs · 2019-03-23T22:22:44Z

@xbauch : thanks for the explanation.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 96e9646).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105574298

tautschnig · 2019-03-24T16:37:03Z

unit/memory-analyzer/gdb_api.cpp

+  {
+    REQUIRE(has_gdb);
+  }
+  return false;


Doesn't this now always return false, and thus effectively disable all the tests?

@tautschnig sorry about this type: should be fixed now.

No functional change. Only wraps the test cases in an if-statements checking the presence of gdb.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 01da544).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105588250

chrisr-diffblue requested review from danpoe and smowton March 22, 2019 14:41

smowton approved these changes Mar 22, 2019

View reviewed changes

smowton reviewed Mar 22, 2019

View reviewed changes

xbauch force-pushed the fix/gdb-api-unit-test branch from 65a30ea to b3c6c40 Compare March 22, 2019 14:50

NlightNFotis approved these changes Mar 22, 2019

View reviewed changes

allredj reviewed Mar 22, 2019

View reviewed changes

peterschrammel reviewed Mar 22, 2019

View reviewed changes

peterschrammel assigned xbauch Mar 22, 2019

xbauch force-pushed the fix/gdb-api-unit-test branch from b3c6c40 to 2af9fff Compare March 23, 2019 06:43

tautschnig reviewed Mar 23, 2019

View reviewed changes

allredj reviewed Mar 23, 2019

View reviewed changes

xbauch force-pushed the fix/gdb-api-unit-test branch from 2af9fff to b20e39c Compare March 23, 2019 20:37

xbauch force-pushed the fix/gdb-api-unit-test branch from b20e39c to 3a1aa7c Compare March 23, 2019 21:45

xbauch force-pushed the fix/gdb-api-unit-test branch from 3a1aa7c to 96e9646 Compare March 24, 2019 09:52

allredj reviewed Mar 24, 2019

View reviewed changes

tautschnig requested changes Mar 24, 2019

View reviewed changes

Add check that gdb is on the path

01da544

No functional change. Only wraps the test cases in an if-statements checking the presence of gdb.

xbauch force-pushed the fix/gdb-api-unit-test branch from 96e9646 to 01da544 Compare March 24, 2019 17:03

tautschnig approved these changes Mar 24, 2019

View reviewed changes

allredj reviewed Mar 24, 2019

View reviewed changes

tautschnig merged commit ccd30dd into diffblue:develop Mar 24, 2019

xbauch deleted the fix/gdb-api-unit-test branch June 10, 2019 12:30

Add check that gdb is on the path #4420

Add check that gdb is on the path #4420

Uh oh!

Conversation

xbauch commented Mar 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

martin-cs commented Mar 23, 2019

Uh oh!

xbauch commented Mar 23, 2019

Uh oh!

tautschnig commented Mar 23, 2019

Uh oh!

martin-cs commented Mar 23, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!