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

Adding e2e tests for pages which use middleware to redirect #10945

Merged
merged 1 commit into from
May 7, 2024

Conversation

codyrancher
Copy link
Contributor

@codyrancher codyrancher commented May 3, 2024

Summary

Also fixed a couple routes, and removed some test setup code that was commented on in another review.

Technical notes summary

I wanted to put these tests in place before changing or removing the middleware to make sure we keep the experience consistent.

Areas or cases that should be tested

These are the tests.

Areas which could experience regressions

None

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

Also removed a couple files that were obviously broken and didn't redirect to anything.
@@ -89,7 +89,7 @@ export const routerOptions = {
name: 'account-create-key'
},
{
path: 'auth',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This configuration wasn't actually being used correctly. It was falling back on /c/:cluster/:product

auth

@@ -99,7 +99,7 @@ export const routerOptions = {
name: 'c-cluster-ecm'
},
{
path: 'settings',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This configuration wasn't actually being used correctly. It was falling back on /c/:cluster/:product

settings

@codyrancher codyrancher added this to the v2.9.0 milestone May 6, 2024
@@ -14,7 +14,7 @@ global.TextEncoder = TextEncoder;
global.TextDecoder = TextDecoder;

Vue.config.productionTip = false;
Vue.use(i18n, { store: { dispatch() {} } });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change in regards to #10923 (comment)

@codyrancher codyrancher marked this pull request as ready for review May 6, 2024 15:39
Copy link
Contributor

@cnotv cnotv left a comment

Choose a reason for hiding this comment

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

For me, the test coverage seems fine, but I do not grasp why it should redirect and if this is common in all these pages, would it not be better a unit test at this point?
It obviously does not exclude having both or any issue to have just E2E, so it's just a simple curiosity.

@codyrancher
Copy link
Contributor Author

For me, the test coverage seems fine, but I do not grasp why it should redirect and if this is common in all these pages, would it not be better a unit test at this point? It obviously does not exclude having both or any issue to have just E2E, so it's just a simple curiosity.

Most of these redirects don't have any logic so I was planning to move them as redirects defined in the router config. This would change the units all together so I'd have to rewrite all the unit tests and therefore I wouldn't be verifying that I didn't change behavior.

I could just make the changes and maybe write some router unit tests without this verification step. I'm okay with either. Let me know which you'd prefer.

@cnotv
Copy link
Contributor

cnotv commented May 6, 2024

Most of these redirects don't have any logic so I was planning to move them as redirects defined in the router config. This would change the units all together so I'd have to rewrite all the unit tests and therefore I wouldn't be verifying that I didn't change behavior.

I would expect just to change the spied function, although I may completely miss the actual case.
Could we at least specify when the redirect happens in the test? Because as it's written it simply redirects for no reason.
Is it me overlooking something or would you agree? If not agree, please leave it as it is :)

I could just make the changes and maybe write some router unit tests without this verification step. I'm okay with either. Let me know which you'd prefer.

I tend to stick to an already done work, especially if is also fine.

Copy link
Contributor

@cnotv cnotv left a comment

Choose a reason for hiding this comment

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

LGTM

@codyrancher codyrancher merged commit 6fad6e7 into rancher:master May 7, 2024
34 checks passed
@codyrancher codyrancher mentioned this pull request May 22, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants