Break the debugger when a test failed #2830

Naios · 2017-08-13T22:56:15Z

Currently only implemented for the MSVC debugger if attached

The main idea is that we instantly see which test failed if we execute a test inside a debugger.

I would like to hear your opinion about this:

Maybe we could allow to enable/disable this from CMake.
Maybe we could move this up to the test macros which could yield a better stack trace

hkaiser · 2017-08-14T00:17:55Z

I like this idea. However, I'd suggest to use the already existing facilities in HPX. See here for an example usage. This code looks up whether there was a command line option --hpx:attach-debugger=exception. I think we could add a --hpx:attach-debugger=test-failure which would enable attaching a debugger for your use case. See here for the relevant spots:

So your code would look like:

if (hpx::get_config_entry("hpx.attach_debugger", "") == "test-failure")
{
    hpx::util::attach_debugger();
}

That would however require to extract the corresponding declaration into a separate header file: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/util/command_line_handling.hpp#L93.

Naios · 2017-08-14T12:07:53Z

@hkaiser This sounds great. I thought it would be comfortable if this would work automaitically without requiring any argument, since many unit tests don't even parse their arguments.
But I also think that one should be able to disable this.

Do you think that there is a solution which solves both ideas?

hkaiser · 2017-08-14T12:45:39Z

@Naios Hmmm, I'd prefer if this was not ON by default. But I agree that many tests don't handle command line options, I didn't think of this...

Internally HPX relies on extracting the information about any possible --hpx:attach-debugger=... option from its internal configuration database (using the key hpx.attach_debugger). This allows to enable extracting this information from the execution environment as well. If we added

"attach_debugger = ${HPX_ATTACH_DEBUGGER}",

here: https://github.com/STEllAR-GROUP/hpx/blob/master/src/util/runtime_configuration.cpp#L197.

Now setting HPX_ATTACH_DEBUGGER=test-failure in the execution environment would trigger the desired functionality. Note that you could still overwrite this by using a command line option if we added off as a valid argument for it (see link posted earlier).

Similarly we could add a compile-time configuration option enabling this by default:

#if defined(HPX_HAVE_TESTS_ATTACH_DEBUGGER_ON_FAILURE)
    "attach_debugger = ${HPX_ATTACH_DEBUGGER:test-failure}",
#else
    "attach_debugger = ${HPX_ATTACH_DEBUGGER}",
#endif

Naios · 2017-08-14T14:38:07Z

@hkaiser Perfect, thanks for your quick reply. That was the implementation I thought about 👍

Naios · 2017-08-21T10:33:33Z

@hkaiser I applied the requested changes:

attach_debugger() was refactored out
The option is now enabled by default if HPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE is set

hkaiser · 2017-08-21T11:12:26Z

hpx/util/lightweight_test.hpp

-                { HPX_ASSERT(false); return; }
-        }
-    }
+    void increment(counter_type c);


Shouldn't this be HPX_EXPORTed?

I thought that too, however lightweight_test.cpp is built into the hpx_init static library and thus we don't need to export the symbol.

Right, thanks!

hkaiser · 2017-08-21T11:12:40Z

hpx/util/lightweight_test.hpp

-                { HPX_ASSERT(false); return 0; }
-        }
-    }
+    std::size_t get(counter_type c) const;


Same here: HPX_EXPORT?

Same as above

hkaiser

LGTM, thanks a lot!

hkaiser · 2017-08-22T14:40:54Z

@Naios: one more thing - may I ask you to add support for --hpx:attach-debugger=off (or similar) allowing to disable the pre-set test-failure if -DHPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE=On?

Naios · 2017-08-22T18:23:21Z

@hkaiser Yes, good idea. I'll add it.

* Set the cmake variable -DHPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE to enable this without to set it inside the config. * Move some methods from the test fixture to its compilation unit.

Naios · 2017-08-27T20:33:22Z

@hkaiser I applied the requested change

hkaiser · 2017-08-27T20:36:49Z

src/util/command_line_handling.cpp

                std::cerr <<
                    "hpx::init: command line warning: --hpx:attach-debugger: "
                    "invalid option: " << option << ". Allowed values are "
-                    "'startup' or 'exception'" << std::endl;
+                    "'off, startup', 'exception' or 'test-failure'" << std::endl;


Nitpick: this misses apostrophes

* Useful when HPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE is set in CMake, and we want to disable this option at runtime.

hkaiser added category: diagnostics type: enhancement labels Aug 13, 2017

hkaiser added this to the 1.1.0 milestone Aug 13, 2017

Naios force-pushed the test_break branch 2 times, most recently from 573c8e7 to ddb53b0 Compare August 13, 2017 23:44

Naios force-pushed the test_break branch 2 times, most recently from 29dee3f to 980b405 Compare August 21, 2017 10:31

hkaiser reviewed Aug 21, 2017

View reviewed changes

hkaiser approved these changes Aug 21, 2017

View reviewed changes

Naios force-pushed the test_break branch 2 times, most recently from 30652a9 to d861187 Compare August 21, 2017 21:06

Break the debugger when a test failed

b1b1deb

* Set the cmake variable -DHPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE to enable this without to set it inside the config. * Move some methods from the test fixture to its compilation unit.

Naios force-pushed the test_break branch from d861187 to 070105e Compare August 27, 2017 20:33

hkaiser reviewed Aug 27, 2017

View reviewed changes

Allow to disable the hpx:attach-debugger option

a3e1cfb

* Useful when HPX_WITH_ATTACH_DEBUGGER_ON_TEST_FAILURE is set in CMake, and we want to disable this option at runtime.

Naios force-pushed the test_break branch from 070105e to a3e1cfb Compare August 27, 2017 20:47

hkaiser merged commit 8acb6a7 into STEllAR-GROUP:master Aug 30, 2017

biddisco mentioned this pull request Oct 4, 2017

Add #include files needed to set _POSIX_VERSION for debug check #2930

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break the debugger when a test failed #2830

Break the debugger when a test failed #2830

Naios commented Aug 13, 2017 •

edited

hkaiser commented Aug 14, 2017 •

edited

Naios commented Aug 14, 2017 •

edited

hkaiser commented Aug 14, 2017 •

edited

Naios commented Aug 14, 2017

Naios commented Aug 21, 2017

hkaiser Aug 21, 2017

Naios Aug 21, 2017

hkaiser Aug 21, 2017

hkaiser Aug 21, 2017

Naios Aug 21, 2017

hkaiser left a comment

hkaiser commented Aug 22, 2017

Naios commented Aug 22, 2017

Naios commented Aug 27, 2017

hkaiser Aug 27, 2017 •

edited

Naios Aug 27, 2017

Break the debugger when a test failed #2830

Break the debugger when a test failed #2830

Conversation

Naios commented Aug 13, 2017 • edited

hkaiser commented Aug 14, 2017 • edited

Naios commented Aug 14, 2017 • edited

hkaiser commented Aug 14, 2017 • edited

Naios commented Aug 14, 2017

Naios commented Aug 21, 2017

hkaiser Aug 21, 2017

Choose a reason for hiding this comment

Naios Aug 21, 2017

Choose a reason for hiding this comment

hkaiser Aug 21, 2017

Choose a reason for hiding this comment

hkaiser Aug 21, 2017

Choose a reason for hiding this comment

Naios Aug 21, 2017

Choose a reason for hiding this comment

hkaiser left a comment

Choose a reason for hiding this comment

hkaiser commented Aug 22, 2017

Naios commented Aug 22, 2017

Naios commented Aug 27, 2017

hkaiser Aug 27, 2017 • edited

Choose a reason for hiding this comment

Naios Aug 27, 2017

Choose a reason for hiding this comment

Naios commented Aug 13, 2017 •

edited

hkaiser commented Aug 14, 2017 •

edited

Naios commented Aug 14, 2017 •

edited

hkaiser commented Aug 14, 2017 •

edited

hkaiser Aug 27, 2017 •

edited