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

Add acceptance tests for pipeline schedule #100

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

chloeruka
Copy link
Contributor

@chloeruka chloeruka commented Feb 17, 2021

This resource also lacked acceptance tests, so in order to guard against regressions we have added the following simple acceptance tests:

  • creating a schedule (with only the required attributes)
  • updating a schedule (e.g. changing its cronline)
  • importing a pipeline schedule

This work required the addition of UUID to the pipeline schedule resource schema. The UUID is used to assemble the slug of an existing pipeline schedule.

@chloeruka chloeruka force-pushed the acceptance-test-pipeline-schedule branch 2 times, most recently from 52eb16b to c8a7988 Compare February 18, 2021 03:29
@chloeruka chloeruka changed the title WIP: Add acceptance tests for pipeline schedule Add acceptance tests for pipeline schedule Feb 18, 2021
@chloeruka chloeruka marked this pull request as ready for review February 18, 2021 03:30
This resource also lacked acceptance tests, so in order to guard against
regressions we have added the following simple acceptance tests:

* creating a schedule (with only the required attributes)
* updating a schedule (e.g. changing its cronline)
* importing a pipeline schedule

This work required the addition of UUID to the pipeline schedule
resource schema. The UUID is used to assemble the slug of an existing
pipeline schedule.
@chloeruka chloeruka force-pushed the acceptance-test-pipeline-schedule branch from c8a7988 to 01c57c3 Compare February 19, 2021 00:31
Config: testAccPipelineScheduleConfigBasic("foo", "0 * * * *"),
Check: resource.ComposeAggregateTestCheckFunc(
// Schedules need a pipeline
testAccCheckPipelineExists("buildkite_pipeline.foobar", &resourcePipeline),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to check the pipeline in these tests, or can we assume it works thanks to the specific tests for the pipeline resource?

Copy link
Contributor Author

@chloeruka chloeruka Feb 22, 2021

Choose a reason for hiding this comment

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

Ahh, yeah this is the rub; we don't really need to know that it exists, but if we want to validate we put a schedule on the correct pipeline later, we need a reference, so we memref it to testAccCheckPipelineExists which populates the (until now nil) value of resourcePipeline

Copy link
Contributor

Choose a reason for hiding this comment

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

oooohhh, of course. I keep forgetting this methods are also about grabbing an in-memory copy of the data 👍

Copy link
Contributor

@yob yob left a comment

Choose a reason for hiding this comment

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

lovely ❤️

I had a couple of non-blocking questions.

@chloeruka chloeruka requested a review from yob February 22, 2021 01:59
Copy link
Contributor

@yob yob left a comment

Choose a reason for hiding this comment

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

looks good to me, I've confirmed the new tests are green when run locally for me 👍

@chloeruka chloeruka merged commit df755d4 into main Feb 22, 2021
@chloeruka chloeruka deleted the acceptance-test-pipeline-schedule branch February 22, 2021 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants