Skip to content
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

Disable streaming by default #770

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Disable streaming by default #770

wants to merge 17 commits into from

Conversation

mtmk
Copy link
Contributor

@mtmk mtmk commented Jul 7, 2024

  • Disable streaming by default since it's deprecated and building with it on Windows is not easy
  • Initial Windows testing with default options (i.e. no valgrind etc.) fixed in benchmark for SubscribeAsync #774

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.65%. Comparing base (1553d4a) to head (8bf5313).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #770      +/-   ##
==========================================
+ Coverage   68.71%   70.65%   +1.94%     
==========================================
  Files          39       47       +8     
  Lines       15207    15136      -71     
  Branches     3143     3094      -49     
==========================================
+ Hits        10449    10695     +246     
+ Misses       1700     1457     -243     
+ Partials     3058     2984      -74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtmk mtmk marked this pull request as ready for review July 8, 2024 18:09
@mtmk mtmk requested a review from levb July 8, 2024 18:09
Copy link
Collaborator

@levb levb left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, but I wonder if we need the auto-retry?

run_ctest
result=$?

# Retry if failed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am 0/5 against auto-retries. They mask out the (annoying) flappers, but I'd rather be annoyed and more motivated to fix them, TBH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately there aren't a definitive set of failing tests but there seems to be an issue with 'something' however much I excluded failing tests, one or two always fails! (I'm guessing nats-server process management, just a guess, btw having similar issues with nats.net tests on windows).

if we removed retires tests almost certain to never pass and hide potential real failures which might be useful. having said that, i see your point as well.

if you still think the same please let me know i can remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this logic applies to Windows only, I'm good with it for now, but would be great to get to the bottom of this issue. There are 2-3 "known" flappers at the moment, but the rest - 2/5 we should aim to pass, maybe with specific exclusions as applicable.

cmake --build build --config Debug

- name: Test
shell: bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

0/5 as a subsequent improvement (separate PR), maybe consolidate this with build-test.yml, so we have a windows and a unix versions, and keep the on-... workflow files shorter/simpler.

# Conflicts:
#	.github/workflows/on-pr-debug.yml
@mtmk mtmk changed the title Windows build tests Disable streaming by default Sep 6, 2024
@mtmk mtmk requested a review from levb September 6, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants