-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
benchmark for SubscribeAsync #774
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #774 +/- ##
==========================================
+ Coverage 68.71% 68.86% +0.14%
==========================================
Files 39 39
Lines 15207 15238 +31
Branches 3143 3153 +10
==========================================
+ Hits 10449 10493 +44
+ Misses 1700 1678 -22
- Partials 3058 3067 +9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
|
refactored bench Added BenchSubscribeAsync_Inject Added BenchSubscribeAsync_InjectSlow
.github/workflows/on-pr-debug.yml
Outdated
@@ -98,4 +106,4 @@ jobs: | |||
- name: Test | |||
run: | | |||
cd build | |||
./bin/Debug/testsuite | |||
ctest -L 'test' --timeout 60 --output-on-failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mtmk ctest
seems to work fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't run any tests
https://github.com/nats-io/nats.c/actions/runs/10116438126/job/27979286329?pr=774#step:5:10
Test project D:/a/nats.c/nats.c/build
No tests were found!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtmk I think I cleaned it up well, borrowing from your PR. Not sure why many tests are flaky, for now retry=3. Please review when you have a chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @mtmk mentioned that the ctest -L
does not seem to be working.
@levb I saw this test |
@kozlovic yes, I think it was caused by nats-io/nats-server#5506, but haven't had a chance to understand/adjust the test yet. If you know how - would MUCH appreciate it! I raised it in the server channel back then, but didn't get much traction. To be clear, it has been flaking for some time now, not this PR in particular. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (looking at the flap test to see if I understand what's going on. If I do, will post PR if this is an issue with the test itself).
@levb Are you going to merge this to main or is it to be merged only onto the other PR? I want to submit a PR for a fix to the test, but wanted to base it on that one so that there is less chance of a conflict? |
No description provided.