refactor(core): Move execution engine code out of n8n-workflow (no-changelog) #12147

netroy · 2024-12-10T22:28:15Z

Summary

This PR moves about 1400 lines related to the execution engine out of n8n-workflow, deletes some dead code and unnecessary indirection, and adds some additional unit tests.

E2E tests
Workflow tests

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

cypress · 2024-12-10T22:40:46Z

n8n Run #8320

Run Properties: Passed #8320 • 451a510420: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*

Project	`n8n`
Branch Review	`move-execution-engine-code`
Run status	`Passed #8320`
Run duration	`04m 48s`
Commit	`451a510420: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*`
Committer	`कारतोफ्फेलस्क्रिप्ट™`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`2`
Pending	`0`
Skipped	`0`
Passing	`480`
View all changes introduced in this branch ↗︎

codecov · 2024-12-10T22:55:26Z

Codecov Report

Attention: Patch coverage is 73.96450% with 88 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/core/src/WorkflowExecute.ts	55.44%	40 Missing and 5 partials ⚠️
packages/cli/src/webhooks/webhook.service.ts	74.60%	10 Missing and 6 partials ⚠️
packages/core/src/TriggersAndPollers.ts	73.68%	10 Missing ⚠️
packages/core/src/ActiveWorkflows.ts	0.00%	4 Missing ⚠️
packages/cli/src/webhooks/waiting-webhooks.ts	40.00%	3 Missing ⚠️
packages/core/src/DirectoryLoader.ts	85.71%	1 Missing and 1 partial ⚠️
...ckages/editor-ui/src/composables/useNodeHelpers.ts	94.87%	2 Missing ⚠️
...hain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts	0.00%	1 Missing ⚠️
packages/cli/src/load-nodes-and-credentials.ts	96.00%	1 Missing ⚠️
...li/src/services/dynamic-node-parameters.service.ts	66.66%	1 Missing ⚠️
... and 3 more

📢 Thoughts on this report? Let us know!

…ne-code

despairblue

I added some questions and suggestions, but all of these can be handled in another PR.

packages/cli/src/webhooks/webhook.service.ts

despairblue · 2024-12-12T10:28:49Z

packages/cli/src/__tests__/active-workflow-manager.test.ts

+		const webhookNode = mock<INode>({ type: 'webhookNode' });
+
+		nodeTypes.getByNameAndVersion.mockImplementation((type) => {
+			// TODO: getByNameAndVersion signature needs to be updated to allow returning undefined


That's not accurate, is it? It throw there if the node can't be found:

n8n/packages/core/src/DirectoryLoader.ts

Lines 188 to 203 in 365e82d

getNode(nodeType: string) {

const {

nodeTypes,

known: { nodes: knownNodes },

} = this;

if (!(nodeType in nodeTypes) && nodeType in knownNodes) {

const { sourcePath } = knownNodes[nodeType];

this.loadNodeFromFile(sourcePath);

}

if (nodeType in nodeTypes) {

return nodeTypes[nodeType];

}

throw new UnrecognizedNodeTypeError(this.packageName, nodeType);

}

You also removed all other checks further up the call chain, so maybe this TODO can be removed and this mock should throw instead of returning undefined?

There are quite a few implementations of INodeTypes that are not consistent with the actual main implementation NodeTypes. I want to clean all of these up in a separate PR.

despairblue · 2024-12-12T10:30:45Z

packages/core/test/WorkflowExecute.test.ts

+
+		const nodeTypes = mock<INodeTypes>();
+		nodeTypes.getByNameAndVersion.mockImplementation((type) => {
+			// TODO: getByNameAndVersion signature needs to be updated to allow returning undefined


Same as the other coment.

packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts

packages/core/test/RoutingNode.test.ts

packages/core/src/RoutingNode.ts

packages/core/test/RoutingNode.test.ts

github-actions · 2024-12-12T11:40:08Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

…ne-code

github-actions · 2024-12-12T12:46:15Z

✅ All Cypress E2E specs passed

janober · 2024-12-19T10:39:28Z

Got released with n8n@1.73.0

netroy added 5 commits December 10, 2024 18:43

refactor(core): Move all webhook related code out of the Workflow class

483b414

move more execution engine code out of workflows package

f247aac

move RoutingNode to core

86219f7

move even more code out of the workflow package

3060ed2

delete some dead code and unnecessary indirection

9b183eb

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Dec 10, 2024

netroy marked this pull request as ready for review December 10, 2024 22:33

netroy added 5 commits December 11, 2024 11:32

Merge remote-tracking branch 'origin/master' into move-execution-engi…

d60f75a

…ne-code

additional tests

80872ed

Merge remote-tracking branch 'origin/master' into move-execution-engi…

88ac757

…ne-code

Merge remote-tracking branch 'origin/master' into move-execution-engi…

57d997b

…ne-code

Merge remote-tracking branch 'origin/master' into move-execution-engi…

2855a89

…ne-code

netroy requested review from ivov and despairblue December 11, 2024 18:25

despairblue previously approved these changes Dec 12, 2024

View reviewed changes

netroy added 2 commits December 12, 2024 13:12

address PR comments

93f5931

Merge remote-tracking branch 'origin/master' into move-execution-engi…

451a510

…ne-code

netroy dismissed despairblue’s stale review via 451a510 December 12, 2024 12:13

despairblue approved these changes Dec 12, 2024

View reviewed changes

netroy merged commit 5a055ed into master Dec 12, 2024
38 checks passed

netroy deleted the move-execution-engine-code branch December 12, 2024 12:54

This comment was marked as off-topic.

Sign in to view

janober added the Released label Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Move execution engine code out of n8n-workflow (no-changelog) #12147

refactor(core): Move execution engine code out of n8n-workflow (no-changelog) #12147

netroy commented Dec 10, 2024 •

edited by despairblue

Loading

cypress bot commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

despairblue left a comment

despairblue Dec 12, 2024

netroy Dec 12, 2024

despairblue Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024

github-actions bot commented Dec 12, 2024

This comment was marked as off-topic.

janober commented Dec 19, 2024

	getNode(nodeType: string) {
	const {
	nodeTypes,
	known: { nodes: knownNodes },
	} = this;
	if (!(nodeType in nodeTypes) && nodeType in knownNodes) {
	const { sourcePath } = knownNodes[nodeType];
	this.loadNodeFromFile(sourcePath);
	}

	if (nodeType in nodeTypes) {
	return nodeTypes[nodeType];
	}

	throw new UnrecognizedNodeTypeError(this.packageName, nodeType);
	}

refactor(core): Move execution engine code out of n8n-workflow (no-changelog) #12147

refactor(core): Move execution engine code out of n8n-workflow (no-changelog) #12147

Conversation

netroy commented Dec 10, 2024 • edited by despairblue Loading

Summary

Review / Merge checklist

cypress bot commented Dec 10, 2024 • edited Loading

n8n Run #8320

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

despairblue left a comment

Choose a reason for hiding this comment

despairblue Dec 12, 2024

Choose a reason for hiding this comment

netroy Dec 12, 2024

Choose a reason for hiding this comment

despairblue Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2024

github-actions bot commented Dec 12, 2024

This comment was marked as off-topic.

janober commented Dec 19, 2024

netroy commented Dec 10, 2024 •

edited by despairblue

Loading

cypress bot commented Dec 10, 2024 •

edited

Loading

codecov bot commented Dec 10, 2024 •

edited

Loading

despairblue Dec 12, 2024 •

edited

Loading