Skip to content

Relax CHECK condition in benchmark_runner.cc#938

Merged
LebedevRI merged 3 commits intogoogle:masterfrom
chfast:skip_with_error
Feb 21, 2020
Merged

Relax CHECK condition in benchmark_runner.cc#938
LebedevRI merged 3 commits intogoogle:masterfrom
chfast:skip_with_error

Conversation

@chfast
Copy link
Copy Markdown
Contributor

@chfast chfast commented Feb 14, 2020

If the benchmark state contains an error, do not expect any iterations has been run.
This allows using SkipWithError() and return early from the benchmark function.

Fixes #937.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.004%) to 92.045% when pulling 4db5fe3 on chfast:skip_with_error into 8982e1e on google:master.

Copy link
Copy Markdown
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
I think this is missing documentation change, to advertise this new possibility.
And a negative test, if there isn't one already (if there is one, point me to it?),

void BM_zzz(benchmark::State& state) {
  // empty
}

@LebedevRI LebedevRI self-assigned this Feb 15, 2020
@LebedevRI
Copy link
Copy Markdown
Collaborator

@chfast ping :)

@chfast
Copy link
Copy Markdown
Contributor Author

chfast commented Feb 20, 2020

And a negative test, if there isn't one already (if there is one, point me to it?),

I was not able to implement this. Are there any examples of tests expecting CHECK() macro failures?

@LebedevRI
Copy link
Copy Markdown
Collaborator

And a negative test, if there isn't one already (if there is one, point me to it?),

I was not able to implement this. Are there any examples of tests expecting CHECK() macro failures?

Hmm. I was thinking of using googletest's death tests, but since there clearly isn't a test
that would test the current assertion, adding one would be cumbersome given how
it would need to be called.

So lets not, lets just add the docs :)

If the benchmark state contains an error, do not expect any iterations has been run.
This allows using SkipWithError() and return early from the benchmark function.
Copy link
Copy Markdown
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for the contribution!

@LebedevRI LebedevRI merged commit c078337 into google:master Feb 21, 2020
antoniovicente added a commit to antoniovicente/envoy that referenced this pull request Jul 24, 2020
…enchmark_test_benchmark_test timeouts under tsan

also, bump up the googlebenchmark version to pickup the fix to SkipWithError, google/benchmark#938

Signed-off-by: Antonio Vicente <avd@google.com>
mattklein123 pushed a commit to envoyproxy/envoy that referenced this pull request Jul 24, 2020
…enchmark_test_benchmark_test timeouts under tsan (#12264)

also, bump up the googlebenchmark version to pickup the fix to SkipWithError, google/benchmark#938

Signed-off-by: Antonio Vicente <avd@google.com>
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
…enchmark_test_benchmark_test timeouts under tsan (envoyproxy#12264)

also, bump up the googlebenchmark version to pickup the fix to SkipWithError, google/benchmark#938

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020
…enchmark_test_benchmark_test timeouts under tsan (envoyproxy#12264)

also, bump up the googlebenchmark version to pickup the fix to SkipWithError, google/benchmark#938

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
@LebedevRI LebedevRI removed their assignment Aug 18, 2020
JBakamovic pushed a commit to JBakamovic/benchmark that referenced this pull request Sep 11, 2020
* Add State::error_occurred()

* Relax CHECK condition in benchmark_runner.cc

If the benchmark state contains an error, do not expect any iterations has been run.
This allows using SkipWithError() and return early from the benchmark function.

* README.md: document new possible usage of SkipWithError()
@chfast chfast deleted the skip_with_error branch March 9, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert failure because of SkipWithError()

4 participants