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

fix: Fix VPC landing page test import #9605

Merged

Conversation

jdamore-linode
Copy link
Contributor

Description 📝

This is a tiny change to fix the TypeScript error we're seeing when running vpc-landing-page.spec.ts. It looks like we did a find+replace of vpc to vpcs which also inadvertently touched the import.

How to test 🧪

Run yarn && yarn build && yarn start:manager:ci, and then run:

yarn cy:run -s "cypress/e2e/core/vpc/vpc-landing-page.spec.ts"

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

Since we've renamed the vpc folder in src/features to be src/features/VPCs and updated routing to be /vpcs, would it make more sense to rename the file to vpcs.ts and change the name of the e2e vpc test folder to vpcs as well?

@jdamore-linode
Copy link
Contributor Author

Since we've renamed the vpc folder in src/features to be src/features/VPCs and updated routing to be /vpcs, would it make more sense to rename the file to vpcs.ts and change the name of the e2e vpc test folder to vpcs as well?

@coliu-akamai That's a good thought! I've never really tried to make the test files correspond too closely to the src files, but that doesn't mean it isn't worth doing.

I think I'd rather merge this fix as-is, just in the interest of getting it fixed quickly before more branches get created off develop, and handle clean-up and reorganization later. I'll write up a ticket to improve consistency between the test and src filenames!

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

🎉

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Aug 30, 2023
@jdamore-linode
Copy link
Contributor Author

Going to merge this even though the tests haven't run. No job got kicked off so the alternative would be to push some dummy change to try to trigger a job, and I don't think it's really worth the effort when the scope of this fix is so small and clear, and there's no risk of introducing any user-facing issues here

@jdamore-linode jdamore-linode merged commit 9380480 into linode:develop Aug 30, 2023
corya-akamai pushed a commit to corya-akamai/manager that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Missing Changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants