-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
refactor(core): Add more unit tests for Workflow.ts
(no-changelog)
#9868
Conversation
…abled nodes 1. ignore a node that would otherwise make the function return true 2. disable a node that would make the function return true if it wasn't disabled
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.
I have one question, the other 2 things I already resolved with a commit.
nodeTypes, | ||
}); | ||
|
||
const issues = workflow.checkReadyForExecution({ startNode: disabledNode.name }); |
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.
You're given a startNode
in multiple tests here, but no destinationNode
.
All these tests would also work if we didn't pass the start node. So I assume it's to be able to test the branch that calls getChildNodes
.
Would it make sense to change one of these tests to pass destinationNode
so that we also cover getParentNodes
?
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.
I think we should add a separate group of tests for destinationNode
.
But, I can do that in the next PR adding tests for Workflow
.
4 flaky tests on run #5690 ↗︎Details:
|
Test | Artifacts | |
---|---|---|
NDV > should not retrieve remote options when required params throw errors |
Screenshots
Video
|
|
NDV > Stop listening for trigger event from NDV |
Screenshots
Video
|
12-canvas.cy.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
Canvas Node Manipulation and Navigation > should preserve connections after rename & node-view switch |
Test Replay
Screenshots
Video
|
2-credentials.cy.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
Credentials > should show multiple credential types in the same dropdown |
Test Replay
Screenshots
Video
|
Review all test suite changes for PR #9868 ↗︎
✅ All Cypress E2E specs passed |
Got released with |
Summary
Extracted out of #9858
Review / Merge checklist