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

Add tests for cycles in directed graphs #325

Merged
merged 7 commits into from
Jul 10, 2023
Merged

Conversation

sbaldu
Copy link
Collaborator

@sbaldu sbaldu commented Jul 4, 2023

Add tests to the GraphTest testcase to check that the hashing function used for the sets of edges doesn't cause problems with directed edges forming a cycle (issue #324)

@github-actions github-actions bot added the test Something about test label Jul 4, 2023
@sbaldu sbaldu requested review from nrkramer and ZigRazor July 4, 2023 09:58
@ghost
Copy link

ghost commented Jul 4, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #325 (a0ce040) into master (4d60bf6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #325   +/-   ##
=======================================
  Coverage   97.24%   97.25%           
=======================================
  Files          54       54           
  Lines        7907     7934   +27     
=======================================
+ Hits         7689     7716   +27     
  Misses        218      218           
Flag Coverage Δ
unittests 97.25% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
test/GraphTest.cpp 100.00% <100.00%> (ø)

@ZigRazor
Copy link
Owner

ZigRazor commented Jul 4, 2023

Sorry @sbaldu , these tests are passed?

@sbaldu
Copy link
Collaborator Author

sbaldu commented Jul 4, 2023

Sorry @sbaldu , these tests are passed?

Yes they both pass

@ZigRazor
Copy link
Owner

ZigRazor commented Jul 4, 2023

what do you think @nrkramer? These tests should demostrate that the issue #324 could be closed, right?

@ZigRazor ZigRazor added the Priority:Critical Priority Label for Critical Issue label Jul 4, 2023
@ZigRazor ZigRazor self-assigned this Jul 4, 2023
@nrkramer
Copy link
Collaborator

nrkramer commented Jul 4, 2023

Looks good. Thanks @sbaldu !! This definitely closes that issue out.

However, I'd like to refer you to #326, which demonstrates that sometimes unique edges are not added.

I closed the directed cyclic edges issue, since it seems the issue I'm running into may be separate.

@ZigRazor
Copy link
Owner

I merge this PR even if the problem reported in #324 and the demostration on PR #326 is not solver.
In any case this PR add some tests, that are correct.

@ZigRazor ZigRazor merged commit 0ae7045 into ZigRazor:master Jul 10, 2023
25 checks passed
@sbaldu
Copy link
Collaborator Author

sbaldu commented Jul 10, 2023

Ok @ZigRazor, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Critical Priority Label for Critical Issue test Something about test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants