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

refactor(core): Refactor cli command tests (no-changelog) #9731

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

netroy
Copy link
Member

@netroy netroy commented Jun 13, 2024

Summary

extracted out of #9696

Review / Merge checklist

  • PR title and summary are descriptive
  • Tests included

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 13, 2024
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

So much cleaner, thank you!

@@ -498,7 +498,7 @@ export class Worker extends BaseCommand {
}

// Make sure that the process does not close
await new Promise(() => {});
if (!inTest) await new Promise(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it there's no other choice but in general we should avoid sprinkling inTest checks in production code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was "temporary" code that Jan had to add, after I broke this in #5452 .
At some point I hope to fix this properly, until then we need this ugly check 😞

});

afterAll(async () => {
await testDb.terminate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - Some testDb calls happen in the suite, some happen here. This dries up tests but might condition us to e.g. forget about testDb.terminate in tests that are not about CLI commands and do not have this convenience? Personally I'd prefer not to switch back and forth to see what the suite is doing. Really minor though.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I wasn't happy about that either, and I'm hoping to clean this up even more in the future. I just wanted to get this change in first, so that I can add unit tests for the rest of the commands.

Copy link
Contributor

✅ No visual regressions found.

Copy link

cypress bot commented Jun 18, 2024

3 flaky tests on run #5547 ↗︎

0 388 0 0 Flakiness 3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n Commit: a1c9ad015d
Status: Passed Duration: 04:45 💡
Started: Jun 18, 2024 8:36 AM Ended: Jun 18, 2024 8:41 AM
Flakiness  5-ndv.cy.ts • 2 flaky tests

View Output Video

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
Flakiness  20-workflow-executions.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Current Workflow Executions > should auto load more items if there is space and auto scroll Test Replay Screenshots Video

Review all test suite changes for PR #9731 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

@netroy netroy merged commit 2d02c73 into master Jun 18, 2024
30 checks passed
@netroy netroy deleted the refactor-command-tests branch June 18, 2024 08:50
@janober
Copy link
Member

janober commented Jun 20, 2024

Got released with [email protected]

1 similar comment
@janober
Copy link
Member

janober commented Jun 20, 2024

Got released with [email protected]

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