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

feat(editor): Update copy: Execute --> Test #8137

Merged
merged 14 commits into from
Jan 5, 2024

Conversation

RicardoE105
Copy link
Contributor

@RicardoE105 RicardoE105 commented Dec 21, 2023

Summary

Title self explanatory

Related tickets and issues

https://linear.app/n8n/issue/ADO-129/update-copy-execute-test

Review / Merge checklist

  • PR title and summary are descriptive. Remember, the title automatically goes into the changelog. Use (no-changelog) otherwise. (conventions)

@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 21, 2023
@RicardoE105 RicardoE105 marked this pull request as ready for review December 22, 2023 12:55
@RicardoE105 RicardoE105 changed the title chore(editor): Update copy: 'Execute' --> 'Test' chore(editor): Update copy: Execute --> Test Dec 22, 2023
@RicardoE105 RicardoE105 changed the title chore(editor): Update copy: Execute --> Test refactor(editor): Update copy: Execute --> Test (no-changelog) Dec 22, 2023
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

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

Copy link

cypress bot commented Dec 22, 2023

2 flaky tests on run #3617 ↗︎

0 327 5 0 Flakiness 2

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 RicardoE105 🗃️ e2e/*
Project: n8n Commit: c858ed9fa5
Status: Passed Duration: 07:09 💡
Started: Jan 5, 2024 3:03 PM Ended: Jan 5, 2024 3:11 PM
Flakiness  17-sharing.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Sharing > should work for admin role on credentials created by others (also can share it with themselves) Screenshots Video
Flakiness  29-templates.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Templates > should save template id with the workflow Screenshots Video

Review all test suite changes for PR #8137 ↗︎

Copy link
Contributor

✅ All Cypress E2E specs passed

Copy link
Contributor

@MiloradFilipovic MiloradFilipovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

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

Copy link
Contributor

@mutdmour mutdmour left a comment

Choose a reason for hiding this comment

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

🚀

@@ -42,7 +42,7 @@
{
"parameters": {},
"id": "551313bb-1e01-4133-9956-e6f09968f2ce",
"name": "When clicking \"Execute Workflow\"",
"name": "When clicking \"Test Workflow\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you need to update the fixtures? the tests should have worked even with the old node names, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they kept breaking because we were using some of then to assert the exact label, so updated it all of them

Copy link
Contributor

Choose a reason for hiding this comment

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

why did they keep breaking? the node names of imported workflows should not change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing display names will not change! I only change it there because for some tests we were using this fixtures and asserting that the node display name was "Execute Node" instead "Test Node" so I changed it on all fixtures to avoid issues. I can revert the changes, and run the e2e tests again and only rename in the fixtures where we assert the new name.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see.. new nodes added in tests would need to be updated.. but not imported fixtures in tests.. it's your call here.. it's fine either way

packages/editor-ui/src/views/NodeView.vue Outdated Show resolved Hide resolved
@mutdmour mutdmour changed the title refactor(editor): Update copy: Execute --> Test (no-changelog) feat(editor): Update copy: Execute --> Test Jan 4, 2024
mutdmour
mutdmour previously approved these changes Jan 4, 2024
Copy link
Contributor

github-actions bot commented Jan 4, 2024

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

mutdmour
mutdmour previously approved these changes Jan 5, 2024
Copy link
Contributor

github-actions bot commented Jan 5, 2024

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

Copy link
Contributor

github-actions bot commented Jan 5, 2024

✅ All Cypress E2E specs passed

@RicardoE105 RicardoE105 merged commit df5d07b into master Jan 5, 2024
25 checks passed
@RicardoE105 RicardoE105 deleted the ado-129-update-copy-execute-test branch January 5, 2024 15:23
@github-actions github-actions bot mentioned this pull request Jan 10, 2024
@Joffcom
Copy link
Member

Joffcom commented Jan 10, 2024

Got released with n8n@1.24.0

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

Successfully merging this pull request may close these issues.

4 participants