-
Notifications
You must be signed in to change notification settings - Fork 370
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
test: [M3-7955] - Cypress integration test to add SSH key via Linode Create #10448
Conversation
Coverage Report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @cliu-akamai! Left some comments for potential clean up, but nothing that impacts the quality or effectiveness of your test (which is great, by the way!)
You have my approval pending yarn changeset
. Thanks Cassie!
@@ -76,6 +76,7 @@ authenticate(); | |||
describe('create linode', () => { | |||
before(() => { | |||
cleanUp('linodes'); | |||
cleanUp('ssh-keys'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanUp('ssh-keys'); |
Very minor nit: This call to cleanUp('ssh-keys')
isn't really necessary since your test mocks the SSH key (no real SSH key is being created, so nothing will need to be cleaned up). However, I really appreciate you setting up the deleteAllTestSSHKeys
util and hooking it up to the cleanUp
function since this will be needed for M3-7956!
Oops, never mind! Should have looked closer, realizing now that this test does actually create an SSH key! The ticket technically called for an integration test but I'm thinking leaving this as-is might be fine
const sshPublicKey = `ssh-rsa e2etestkey${randomKey} e2etest@linode`; | ||
const linodeLabel = randomLabel(); | ||
const region: Region = getRegionById('us-southeast'); | ||
const diskLabel: string = 'Debian 10 Disk'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a lot of this mock data is held over from other tests, and we can get rid of them for this test just to make things a little cleaner! Locally, I was able to get rid of all the mock data and setup for disks, VLANs, VPCs & subnets, configs, and the mocks for the Linode DC-specific pricing types and your test still ran really nicely and passed!
Not a super big deal since your test is still testing all of the important things we need to cover for SSH keys 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create-linode.spec.ts
passes ✅ (it didn't for me locally due to a timeout, but it passes remotely)
Code review ✅
Approved pending the addition of a changeset & resolution of the merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Cassie!
Description 📝
Add integration test to add SSH key via Linode create.
Major Changes 🔄
How to test 🧪