-
Notifications
You must be signed in to change notification settings - Fork 14
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
chore: add tests for sshkey (resource + data) #94
Conversation
92d1827
to
c71a999
Compare
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 👍
Left you some comments ✍️
Check: resource.ComposeAggregateTestCheckFunc( | ||
resource.TestCheckResourceAttr(resourceFullName, "id", sshKey.Id), | ||
resource.TestCheckResourceAttr(resourceFullName, "name", sshKey.Name), | ||
resource.TestCheckResourceAttr(resourceFullName, "value", sshKey.Value), |
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.
@razbensimon So the decrypted ssh key exists on the returned object? 🤔
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.
this is the resource
, not the data
tests
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.
Yeah but still, I get that it should be a part of the SshKeyCreatePayload
but I'm not sure it should be a part of the ssh key schema because nobody uses it and it's a secret value. WDYT? (shouldn't be on this PR btw)
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.
put sensitive for now, and lets talk with the team
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.
"testing" | ||
) | ||
|
||
func TestUnitSshKeyDataSourceById(t *testing.T) { |
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.
It really feels like we can DRY those two test functions. I think it's worth an effort 🙏
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.
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.
env0/data_sshkey_test.go
Outdated
} | ||
|
||
runUnitTest(t, testCase, func(mock *client.MockApiClientInterface) { | ||
mock.EXPECT().SshKeys().AnyTimes().Return([]client.SshKey{sshKey}, nil) |
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.
Can we try to be more specific with .Times(1)
?
This is also relevant for all other tests
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.
20f7ef7
worked only for resource tests file. data must have AnyTimes
from some reason
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.
Can you tell maybe how many times it runs? (you can check out using debugger).
I'm asking because it's important to understand this tho.If it's only two - we can add a setting that prevents the second time from running so we'd be able to assert the number of calls easily.
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.
where do you see it on the debugger? what line
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.
🤙
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.
env0/resource_sshkey_test.go
Outdated
} | ||
|
||
runUnitTest(t, testCase, func(mock *client.MockApiClientInterface) { | ||
mock.EXPECT().SshKeyCreate(client.SshKeyCreatePayload{Name: sshKey.Name, Value: sshKey.Value}).Times(1).Return(sshKey, nil).Return(sshKey, nil) |
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.
@razbensimon You got a redundant Return()
call here 🙊
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.
Issue & Steps to Reproduce / Feature Request
Solution