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.

Sign up for GitHub

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

Jump to bottom

Refactoring, fixes, and tests for the graph package #98

Merged
merged 56 commits into from
Feb 24, 2023
Merged

Refactoring, fixes, and tests for the graph package #98

merged 56 commits into from
Feb 24, 2023

Conversation

mattchorlian
Copy link
Collaborator

@mattchorlian mattchorlian commented Apr 26, 2022 •
edited by lhstrh
Loading

This pull request adds unit tests and fixes several bugs.

@marcusrossel
Copy link
Collaborator

marcusrossel commented Jun 1, 2022 •
edited
Loading

Hey, I fixed the cycle-checking issues (locally).
How can I integrate the changes into the project?
Should I just push the commit to this branch (for which I would need write access), or create a new PR?

@lhstrh
Copy link
Member

lhstrh commented Jun 1, 2022

Awesome! Why not push to this branch and then we can go on to get this PR into a state to where it can be merged :-)

Wonseo-C and others added 24 commits June 27, 2022 20:03
@Wonseo-C
Changing Error Message
16c835e
@Wonseo-C
Modify the connect function's Error.
17b855f
@Wonseo-C
Change expected error message
2af403a
@Wonseo-C
modifying reachableOrigins
112a60b
@Wonseo-C
delete uncessary condition
e1928bc
@Wonseo-C
modify test case
166a3fb
@Wonseo-C
modify reactor.ts : canConnect and findOwnOutputs
785550d
@Wonseo-C
Modify the graph test
e43a2f4
@Wonseo-C
Delete hasPriorityOver's undefined return type
e2ed3b6
@Wonseo-C
add removeNode test
289e56c
@CloverCho
Add test for add/remove edges in dependencyGraph
ae74255
@Wonseo-C
removeNode
9ccd1fa
@CloverCho
Add test for toString() of dependencyGraph
ea994fd
@Wonseo-C
sortable dependency graph progress 1
fef5bfc
@CloverCho
Add test for reachableOrigins() function of dependencyGraph
41b37ba
@Wonseo-C
complete sortable dependency graph test
0372b70
@Wonseo-C
Finish graph test!
f8e804a
@Wonseo-C
reset package-lock.json
2dd83d5
@Wonseo-C
new line
9492f17
@Wonseo-C
Modifying about Nit
9019da3
@Wonseo-C
Modifying about Nit - 2
b6d7b9c
@Wonseo-C
clean comment
1f5caf8
@Wonseo-C
Delete not need an import function
9ce99c4
@Wonseo-C
Delete unnecessary print
a7cf7ef
@lhstrh lhstrh marked this pull request as ready for review January 21, 2023 01:46
@lhstrh lhstrh requested a review from hokeun January 21, 2023 01:46
@lhstrh
Copy link
Member

lhstrh commented Jan 21, 2023

For historical reasons, this PR is a bit of a hodgepodge, but with some careful review and all the tests passing, we should be able to merge this into master.

lhstrh
lhstrh reviewed Jan 23, 2023
View reviewed changes
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Found two problems with the tests. Once resolved, we can go ahead and merge this.

__tests__/CausalityGraph.test.ts Outdated Show resolved Hide resolved
__tests__/SimpleMutation.test.ts Outdated Show resolved Hide resolved
__tests__/InvalidMutations.ts Show resolved Hide resolved
__tests__/SimpleMutation.test.ts Outdated Show resolved Hide resolved
src/core/graph.ts Outdated Show resolved Hide resolved
hokeun and others added 15 commits January 23, 2023 14:26
@hokeun @lhstrh
Apply suggestions from code review
81f0ec4 
Co-authored-by: Marten Lohstroh <[email protected]>
@hokeun @lhstrh
Apply suggestions from code review
4c0991f 
Co-authored-by: Marten Lohstroh <[email protected]>
@Wonseo-C
Merge branch 'master' into test-coverage
297e586
@Wonseo-C @lhstrh
Update src/core/graph.ts
b048e2d 
Co-authored-by: Marten Lohstroh <[email protected]>
@Wonseo-C
change the test and reactor.ts: error message
961a125
@Wonseo-C
Merge branch 'test-coverage' of https://github.com/lf-lang/reactor-ts …
ff8cc21 
…into test-coverage
@Wonseo-C
change the expected error
63d5013
@Wonseo-C
change the test case of direct feed through
638fd86
@Wonseo-C
change the expected error message
fcd1014
@Wonseo-C
modify the logic of canconnect
58705b7
@Wonseo-C
change the expected error message
62b751a
@Wonseo-C
change the expected error for SingleEvent test
633f4a9
@Wonseo-C
redesign the CausalityGraph test
0263a42
@Wonseo-C
modify the InvalidMutation test case
1118bcf
@Wonseo-C
add fixme
f87e921
@lhstrh lhstrh merged commit 863bc66 into master Feb 24, 2023
@lhstrh lhstrh deleted the test-coverage branch February 24, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wonseo-C Wonseo-C Wonseo-C left review comments

@lhstrh lhstrh lhstrh approved these changes

@hokeun hokeun Awaiting requested review from hokeun

@petervdonovan petervdonovan Awaiting requested review from petervdonovan

Assignees
No one assigned
Labels
bugfix refactoring testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mattchorlian @marcusrossel @lhstrh @Wonseo-C @CloverCho @hokeun