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

Fix EdgeHolder from incorrectly reporting an active connection #402

Merged
merged 4 commits into from
Sep 28, 2023

Conversation

dagardner-nv
Copy link
Contributor

Description

  • Prevents check_active_connection from mistakenly returning true for a holder where init_owned_edge has been called but neither the init_connected_edge method or the add_connector method have not been called.

Relates to issue #360

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

…nts check_active_connection from mistakenly returning true for a holder where init_owned_edge has been called but neither the init_connected_edge method or the add_connector method have not been called
@dagardner-nv dagardner-nv requested a review from a team as a code owner September 28, 2023 21:18
@dagardner-nv dagardner-nv self-assigned this Sep 28, 2023
@dagardner-nv dagardner-nv added bug Something isn't working non-breaking Non-breaking change labels Sep 28, 2023
cpp/mrc/tests/test_edges.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #402 (8e90892) into branch-23.11 (8b20469) will increase coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-23.11     #402      +/-   ##
================================================
+ Coverage         73.59%   73.62%   +0.03%     
================================================
  Files               385      385              
  Lines             13614    13613       -1     
  Branches           1028     1027       -1     
================================================
+ Hits              10019    10023       +4     
+ Misses             3595     3590       -5     
Flag Coverage Δ
cpp 69.70% <ø> (+0.02%) ⬆️
py 42.09% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cpp/mrc/include/mrc/edge/edge_holder.hpp 79.72% <ø> (-0.28%) ⬇️

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b20469...8e90892. Read the comment docs.

@dagardner-nv
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit ba483b1 into nv-morpheus:branch-23.11 Sep 28, 2023
12 checks passed
rapids-bot bot pushed a commit to nv-morpheus/Morpheus that referenced this pull request Aug 2, 2024
… raise an exception rather than segfaulting (#1829)

* MRC PR nv-morpheus/MRC#402 fixed this issue, this PR adds some unittests to ensure this.
* Update `Pipeline.build_and_start` to re-raise any exceptions raised during build, this prevents `Pipeline.join` from being called which avoids a failed assert in `join` since the expected future was not created.

Closes #953 

## By Submitting this PR I confirm:
- I am familiar with the [Contributing Guidelines](https://github.com/nv-morpheus/Morpheus/blob/main/docs/source/developer_guide/contributing.md).
- When the PR is ready for review, new or existing tests cover these changes.
- When the PR is ready for review, the documentation is up to date with these changes.

Authors:
  - David Gardner (https://github.com/dagardner-nv)

Approvers:
  - Michael Demoret (https://github.com/mdemoret-nv)

URL: #1829
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants