Conversation
paddycarver
force-pushed
the
paddy_serve
branch
2 times, most recently
from
9db2fa0 to
544fa81
Compare
|
OK, let's address the elephant in the room on this one: testing. The testing here is abysmal. I added tests for things I thought it made sense to unit test, but a lot of this PR is just integrating previously built units. I really believe that integration tests are probably our best bet here for creating tests that are actually valuable to us. I think #19 is the right way to address this specific PR, whether that's with terraform-provider-corner or built-in integration tests. My gut says that we probably want to invest in terraform-provider-corner and automate running integration tests separately from the unit tests, but I don't actually know that to be the case. @kmoe, @bflad, any thoughts, opinions, or feelings on the matter? Other than tests, I think this is ready for review. |
paddycarver
force-pushed
the
paddy_serve
branch
2 times, most recently
from
e7f38c4 to
39c0226
Compare
kmoe
left a comment
kmoe
left a comment
There was a problem hiding this comment.
This sort of comprehensive testing is the ideal basis for the new framework. Delighted that we can start serving providers - LGTM!
|
|
||
| got, err := IsCreate(context.Background(), tc.req, tc.typ) | ||
| if err != nil { | ||
| if tc.expectedErr == "" { |
There was a problem hiding this comment.
It doesn't look like expectedErr is used, although I'm happy to leave this in in case the test cases get more complex in future...
There was a problem hiding this comment.
Yeah, sorry, I mostly just wanted to have it so adding tests that provoke errors is easy. I know YAGNI, but tests are one of the places where I like to try and predict future needs, because making adding future cases easy means more comprehensive testing, usually. So when I do table-driven tests, I try to make sure all the non-context.Context arguments can be specified and all the returned values can be checked. (I'm not always perfect at it, but it's a general habit at this point.)
This required updating some of the request/response types. It also required modifying how things implement ApplyTerraform5AttributePathStep, so we could get at the schema.Attribute instead of just the attr.Type. All tests still pass.
Also, delete should get the prior state, not the config, as the config is guaranteed to be nil.
Also, add a nicer error if nil accidentally gets passed to State.Set.
I could've sworn we fixed this in plugin-go, but whatever. Let's be safe.
Add unit tests covering the internal/proto6 package.
A little less repetition.
Introduce ProviderMeta to ReadResourceRequest, check it's being passed in the tests. Split serve tests into multiple files, they're getting impossible to navigate. Finish testing read resource for test_one resource, still need to test it with test_two resource.
Just testing for test_one, with no provider_meta support yet.
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
2 participants
With all this type work going on, it doesn't mean anything if it's not a gRPC server. So let's wire that up.