Improve test failure messages generated by HistogramTester |
|
Issue description
All EXPECT-checking methods from HistogramTester [1] generate failure messages that point to its own code making it hard to translate that to the call point in the failing test case. For instance:
```
[ RUN ] OfflinePageStorageManagerTest.TestClearMultipleTimes
../../base/test/histogram_tester.cc:178: Failure
Expected: expected_count
Which is: 3
To be equal to: actual_count
Which is: 2
Histogram "OfflinePages.ClearStoragePreRunUsage.temporary_namespace_1day" does not have the right number of samples (3) in the expected bucket (50). It has (2).
```
Even though the failure happened inside the OfflinePageStorageManagerTest.TestClearMultipleTimes test the location presented (always) points to base/test/histogram_tester.cc.
I think we should either:
* Add documentation to histogram_tester.h explaining a good code pattern to make failure messages clearer. One idea would be suggesting the use of SCOPED_TRACE but hopefully there's something better.
* Change the HistogramTester API so that it naturally presents better error messages. A probably bad idea would be to add macros that wrap the existing method calls but provide better error messages.
[1] https://cs.chromium.org/chromium/src/base/test/histogram_tester.h
[2] https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#adding-traces-to-assertions
,
Oct 20 2017
I don't think they would as stack traces don't mention line numbers, only method names and files which we already get from the test failure messages.
,
Oct 20 2017
Hmm, I would definitely expect the stack traces to mention line numbers.
,
Oct 21 2017
I think that's platform dependent. IIRC Android does print line numbers (after de-obfuscation) but Linux stack traces look like this for me: [ RUN ] OfflinePageStorageManagerTest.TestClearPagesLessThanLimit [86876:86876:1020/171838.615670:22708187168:ERROR:histogram_tester.cc(41)] #0 0x7f540a649dbd base::debug::StackTrace::StackTrace() #1 0x7f540a6481ec base::debug::StackTrace::StackTrace() #2 0x000006cb3e76 base::HistogramTester::ExpectUniqueSample() #3 0x000004a4338a offline_pages::OfflinePageStorageManagerTest_TestClearPagesLessThanLimit_Test::TestBody() #4 0x000005432f5e testing::internal::HandleSehExceptionsInMethodIfSupported<>() #5 0x0000054299f2 testing::internal::HandleExceptionsInMethodIfSupported<>() #6 0x000005414356 testing::Test::Run() #7 0x000005414ead testing::TestInfo::Run() #8 0x00000541590c testing::TestCase::Run() #9 0x00000542153c testing::internal::UnitTestImpl::RunAllTests() #10 0x000005432fee testing::internal::HandleSehExceptionsInMethodIfSupported<>() #11 0x00000542ad42 testing::internal::HandleExceptionsInMethodIfSupported<>() #12 0x00000542117a testing::UnitTest::Run() #13 0x000006cc9041 RUN_ALL_TESTS() #14 0x000006cc6b5e base::TestSuite::Run() #15 0x00000784697d content::UnitTestTestSuite::Run() #16 0x000002accbc2 _ZN4base8internal13FunctorTraitsIMN7history13HistoryDBTaskEFvvEvE6InvokeINSt3__110unique_ptrIS3_NS8_14default_deleteIS3_EEEEJEEEvS5_OT_DpOT0_ #17 0x000002accb24 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN7history13HistoryDBTaskEFvvEJNSt3__110unique_ptrIS5_NSA_14default_deleteIS5_EEEEEEEvOT_DpOT0_ #18 0x000005fa9c20 _ZN4base8internal7InvokerINS0_9BindStateIMN7content17UnitTestTestSuiteEFivEJNSt3__110unique_ptrIS4_NS7_14default_deleteIS4_EEEEEEEFivEE7RunImplIRKS6_RKNS7_5tupleIJSB_EEEJLm0EEEEiOT_OT0_NS7_16integer_sequenceImJXspT1_EEEE #19 0x000005fa9b6c _ZN4base8internal7InvokerINS0_9BindStateIMN7content17UnitTestTestSuiteEFivEJNSt3__110unique_ptrIS4_NS7_14default_deleteIS4_EEEEEEEFivEE3RunEPNS0_13BindStateBaseE #20 0x00000234478d _ZNKR4base17RepeatingCallbackIFvvEE3RunEv #21 0x000006ccd697 base::(anonymous namespace)::LaunchUnitTestsInternal() #22 0x000006ccd505 base::LaunchUnitTests() #23 0x0000042c52c9 main #24 0x7f53f437ff45 __libc_start_main #25 0x000001beb029 <unknown> From HistogramTester::ExpectUniqueSample This was generated adding a call to base::debug::StackTrace::StackTrace() inside base::HistogramTester::ExpectUniqueSample() and running the OfflinePageStorageManagerTest.TestClearPagesLessThanLimit test.
,
Oct 23 2017
Odd. I wonder whether that has to do with your compilation settings on Linux? Or, maybe Linux really doesn't generate line numbers, and we'll want something extra beyond the stack trace. |
|
►
Sign in to add a comment |
|
Comment 1 by isherman@chromium.org
, Oct 20 2017