-
Notifications
You must be signed in to change notification settings - Fork 6
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 layout bug and add tests #108
Conversation
phschaad
commented
Jul 14, 2023
•
edited
Loading
edited
- Adds tests using the testing framework Jest
- Tests the integrated graph library
- Adds some tests for the vertical layout (specifically, the ranking phase)
- Fixes a few bugs detected through testing, including mishandling of self-loops
- Ensure the vertical layout doesn't get stuck if it fails due to some reason
Codecov Report
@@ Coverage Diff @@
## master #108 +/- ##
=========================================
Coverage ? 90.73%
=========================================
Files ? 7
Lines ? 820
Branches ? 155
=========================================
Hits ? 744
Misses ? 59
Partials ? 17 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Looks great, I only have some questions and a test request. Great work with the testing infrastructure!
} | ||
|
||
public get length(): number { | ||
return this._nodes.size; |
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.
Please explain
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.
Added comment
@@ -121,7 +125,7 @@ export class Graph<NodeT, EdgeT> implements GraphI<NodeT, EdgeT> { | |||
} | |||
|
|||
public numberOfEdges(): number { | |||
return this.edges().length; | |||
return this.edges().length / 2; |
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.
Again, explain
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.
Added comment
test( | ||
'Nested loops with fused assignment and condition edges', | ||
testNestedLoopsFusedEdes | ||
); |
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.
Please add a test for a self edge loop
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.
Added
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!