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

chore: Create table and typeorm entity for tests (no-changelog) #11505

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

burivuhster
Copy link
Contributor

Summary

This PR introduces a new table for storing Tests.
This is part of a larger initiative to enhance our workflow evaluation capabilities.

Changes

  • Add new typeorm entity for Tests
  • Implement DB migration

Related Linear tickets, Github issues, and Community forum posts

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)

@burivuhster burivuhster marked this pull request as ready for review November 1, 2024 14:20
@burivuhster burivuhster requested a review from a team as a code owner November 1, 2024 14:20
@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Nov 1, 2024
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

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

Looks mostly good 👍 Couple comments / questions

@Entity()
@Index(['workflow'])
@Index(['evaluationWorkflow'])
export class TestEntity extends WithTimestamps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Test is very overloaded term. Could we use something more specific? E.g. WorkflowTest? Also could we add a short description for this entity

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what we should name this, but I'm certain that calling this Test* would be confusing for many people.

Even WorkflowTest is overloaded, as it can already either refer to the special test type we have nodes-base, as well as the test-workflows repo.

name: string;

@RelationId((test: TestEntity) => test.workflow)
workflowId: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't he id of WorkflowEntity a string?

workflow: WorkflowEntity;

@RelationId((test: TestEntity) => test.evaluationWorkflow)
evaluationWorkflowId: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well, shouldn't it be string?

evaluationWorkflowId: number;

@ManyToOne('WorkflowEntity', 'evaluationTests')
evaluationWorkflow: WorkflowEntity;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have some documentation what this relation means?

Comment on lines +22 to +29
.withForeignKey('evaluationWorkflowId', {
tableName: 'workflow_entity',
columnName: 'id',
})
.withForeignKey('annotationTagId', {
tableName: 'annotation_tag_entity',
columnName: 'id',
}).withTimestamps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that these don't have onDelete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two references are nullable, and we don't want to delete a test when an evaluation workflow or specific annotation tag being deleted. So the right setting should be ON DELETE SET NULL I guess?

Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

Whatever we decide to rename the entity to, we should update the PR title accordingly, so that when any of of visit this PR in the future, the word tests isn't confusing 🙏🏽.

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/cli/src/databases/entities/test-entity.ts 80.00% 0 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants