-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add unit testing to the provider #662
Conversation
(cd provider && go build $(PULUMI_PROVIDER_BUILD_PARALLELISM) -o $(WORKING_DIR)/bin/$(PROVIDER) -ldflags "-X $(PROJECT)/$(VERSION_PATH)=$(VERSION)#{{if .Config.providerVersion}}# -X #{{ .Config.providerVersion }}#=$(VERSION)#{{end}}#" $(PROJECT)/$(PROVIDER_PATH)/cmd/$(PROVIDER)) | ||
|
||
test: | ||
cd examples && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h | ||
|
||
test_provider: tfgen |
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've tested this without tfgen dependency on pulumi-azuread, but it breaks because of go:embed files. That's really not great. I wonder if we could fix that up so that code compiles and is able to test itself even if generated files are not present yet. For now leaving this in.
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.
Which go:embed
files are missing? I don't think we had that problem with pulumi-aws. We should definitely make sure we don't need the : tfgen
long term.
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 it was the bridge-metadata one.
(cd provider && go build $(PULUMI_PROVIDER_BUILD_PARALLELISM) -o $(WORKING_DIR)/bin/$(PROVIDER) -ldflags "-X $(PROJECT)/$(VERSION_PATH)=$(VERSION)#{{if .Config.providerVersion}}# -X #{{ .Config.providerVersion }}#=$(VERSION)#{{end}}#" $(PROJECT)/$(PROVIDER_PATH)/cmd/$(PROVIDER)) | ||
|
||
test: | ||
cd examples && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h | ||
|
||
test_provider: tfgen | ||
@echo "" | ||
@echo "== test_provider ===================================================================" |
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.
Without the visual marker really easy to get lost in the tfgen noise.
(cd provider && go build $(PULUMI_PROVIDER_BUILD_PARALLELISM) -o $(WORKING_DIR)/bin/$(PROVIDER) -ldflags "-X $(PROJECT)/$(VERSION_PATH)=$(VERSION)#{{if .Config.providerVersion}}# -X #{{ .Config.providerVersion }}#=$(VERSION)#{{end}}#" $(PROJECT)/$(PROVIDER_PATH)/cmd/$(PROVIDER)) | ||
|
||
test: | ||
cd examples && go test -v -tags=all -parallel $(TESTPARALLELISM) -timeout 2h | ||
|
||
test_provider: tfgen |
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.
Which go:embed
files are missing? I don't think we had that problem with pulumi-aws. We should definitely make sure we don't need the : tfgen
long term.
@@ -119,12 +119,18 @@ install_plugins: .pulumi/bin/pulumi | |||
lint_provider: provider | |||
cd provider && golangci-lint run -c ../.golangci.yml | |||
|
|||
provider: tfgen install_plugins | |||
provider: tfgen install_plugins test_provider |
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 seems inverted. I generally expect that test_provider: provider
but not vice versa.
Instead of making provider
depend on test_provider
, can we just add a step in run-acceptence-tests.yml
(probably prerequisites
) that calls make test_provider
?
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.
The reasoning here is that provider
builds a binary from go sources. Those sources better be tested first.
can we just add a step in run-acceptence-tests.yml
Sad here that run-acceptance-tests.yml is duplicated N ways. I'd have to refactor it first to pull out the common bits. I can give this a look.
FYI: If you want to get tests working "right now", you can add the extra test to |
This simpler version seems to work in: https://github.com/pulumi/pulumi-azuread/actions/runs/6437059888/job/17481565040#step:10:37 By running tests after building as a separate step, this bypasses the need to model the dependency in Make. A bit awkward but the least evil quick option at the moment it seems like. Locally, you just have to "know" you need to run tfgen before. @iwahbe another look? |
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.
LGTM
Nit: It would be great to add a comment in the Makefile
explaining that test_provider
needs tfgen
, but we haven't modeled the dependency because of performance reasons.
I think in the ideal state it really should not need tfgen. There is the issue with |
Fixes #661
The reason I'm working on this now is that I'd like to really move upgrade tests out of examples and under provider/ in AWS (and soon GCP). pulumi/pulumi-aws#2855