chore: Refactor integration tests #2986

stevehipwell · 2025-12-08T22:43:53Z

Resolves #2983
Resolves #2984
Resolves #2985
Closes #2941

Before the change?

The acceptance tests are complicated and some are broken
Some of the code is incorrect

After the change?

Acceptance tests have been refactored
- Fewer tests
- Simpler test structure
- Test env setup has been made more granular and documented
Acceptance tests have been run
- Some tests that require paid features haven't been run yet

Pull request checklist

Schema migrations have been created if needed (example)
Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes
No

github-actions · 2025-12-08T22:44:01Z

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

deiga

Half-way through, looking good so far!

deiga · 2025-12-09T06:39:36Z

github/data_source_github_branch_test.go

+	// Can't test due to SDK and test framework limitations
+	// t.Run("queries an invalid branch without error", func(t *testing.T) {
+	// 	randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
+
+	// 	config := fmt.Sprintf(`
+	// 		resource "github_repository" "test" {
+	// 		  name = "tf-acc-test-%[1]s"
+	// 			auto_init = true
+	// 		}
+
+	// 		data "github_branch" "test" {
+	// 			repository = github_repository.test.name
+	// 			branch = "xxxxxx"
+	// 		}
+	// 	`, randomID)
+
+	// 	check := resource.ComposeTestCheckFunc(
+	// 		resource.TestCheckResourceAttr(
+	// 			"data.github_branch.test", "ref", "",
+	// 		),
+	// 	)
+
+	// 	resource.Test(t, resource.TestCase{
+	// 		PreCheck:          func() { skipUnauthenticated(t) },
+	// 		ProviderFactories: providerFactories,
+	// 		Steps: []resource.TestStep{
+	// 			{
+	// 				Config: config,
+	// 				Check:  check,
+	// 			},
+	// 		},
+	// 	})
+	// })


question: Is there value in keeping this test? The API for refs returns 404 if a ref doesn't exist. I don't understand why this behaviour would be expected from the provider

The question about the behaviour is why I've left them commented out. I think we'll need a few issues opened once this is merged to make iterative improvements.

deiga · 2025-12-09T06:54:15Z

github/data_source_github_ref_test.go

+	// Can't test due to SDK and test framework limitations
+	// t.Run("queries an invalid ref without error", func(t *testing.T) {
+	// 	randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
+	// 	config := fmt.Sprintf(`
+	// 		resource "github_repository" "test" {
+	// 		  name = "tf-acc-test-%[1]s"
+	// 			auto_init = true
+	// 		}
+
+	// 		data "github_ref" "test" {
+	// 			repository = github_repository.test.id
+	// 			ref = "heads/xxxxxx"
+	// 		}
+	// 	`, randomID)
+
+	// 	check := resource.ComposeTestCheckFunc(
+	// 		resource.TestCheckNoResourceAttr(
+	// 			"data.github_ref.test", "id",
+	// 		),
+	// 	)
+
+	// 	resource.Test(t, resource.TestCase{
+	// 		PreCheck:          func() { skipUnauthenticated(t) },
+	// 		ProviderFactories: providerFactories,
+	// 		Steps: []resource.TestStep{
+	// 			{
+	// 				Config: config,
+	// 				Check:  check,
+	// 			},
+	// 		},
+	// 	})
+	// })


Same as in https://github.com/integrations/terraform-provider-github/pull/2986/files#r2601299558

deiga · 2025-12-09T07:05:01Z

github/data_source_github_repository_file_test.go

+	// Can't test due to SDK and test framework limitations
+	// t.Run("try reading a non-existent file without an error", func(t *testing.T) {
+	// 	randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
+	// 	config := fmt.Sprintf(`
+	// 		resource "github_repository" "test" {
+	// 			name      			= "tf-acc-test-%s"
+	// 			auto_init 			= true
+
+	// 			lifecycle {
+	// 				ignore_changes = all
+	// 			}
+	// 		}
+
+	// 		data "github_repository_file" "test" {
+	// 			repository     = github_repository.test.name
+	// 			file           = "test"
+	// 		}
+	// 	`, randomID)
+
+	// 	resource.Test(t, resource.TestCase{
+	// 		PreCheck:          func() { skipUnauthenticated(t) },
+	// 		ProviderFactories: providerFactories,
+	// 		Steps: []resource.TestStep{
+	// 			{
+	// 				Config: config,
+	// 				Check: resource.ComposeTestCheckFunc(
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "id"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "branch"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "ref"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "content"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "sha"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "commit_sha"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "commit_message"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "commit_author"),
+	// 					resource.TestCheckNoResourceAttr("data.github_repository_file.test", "commit_email"),
+	// 				),
+	// 			},
+	// 		},
+	// 	})
+	// })


question: Similar to https://github.com/integrations/terraform-provider-github/pull/2986/files#r2601299558, is there value in keeping this as it's not something the API supports?

deiga · 2025-12-09T07:09:08Z

github/data_source_github_rest_api_test.go

+	// t.Run("fails for invalid endpoint", func(t *testing.T) {
+	// 	config := `
+	// 		data "github_rest_api" "test" {
+	// 			endpoint = "/xxx"
+	// 		}
+	// 	`
+
+	// 	resource.Test(t, resource.TestCase{
+	// 		PreCheck:          func() { skipUnauthenticated(t) },
+	// 		ProviderFactories: providerFactories,
+	// 		Steps: []resource.TestStep{
+	// 			{
+	// 				Config:      config,
+	// 				ExpectError: regexp.MustCompile("Error: GET https://api.github.com/xx.*: 414"),
+	// 			},
+	// 		},
+	// 	})
+	// })


question: Why is this commented out?

The current REST API implementation swallows 404 errors, and I can't off the top of my head think of a way to repeatedly get a different error response with an anonymous call. This is another behaviour that needs discussing.

deiga · 2025-12-09T07:12:53Z

github/data_source_github_team_test.go


-		t.Run("with an organization account", func(t *testing.T) {
-			testCase(t, organization)
+			depends_on = ["github_repository.test", "github_team.test", "github_team_repository.test"]


question: Do all of these dependents need to be set? I would assume that github_team_repository would already depend on github_team and github_repository implicitly.

If it doesn't should that be fixed?

Also: I don't think resources can be quoted there

Suggested change

depends_on = ["github_repository.test", "github_team.test", "github_team_repository.test"]

depends_on = [github_team_repository.test]

The depends on needs to be set as it's a data source under test so the resources need to be applied first. Given the behaviour of depends on it's good practice to be explicit here as if the pattern is re-used in a multi step test not having all the resources in there could cause an issue or require additional work to validate if it's the reason for a failure.

Also: I don't think resources can be quoted there

At first glance I'd have agreed with you (if I didn't know the tests were passing), but HCL is very permissive and TF has a lot of legacy baggage.

I'll update these ones, but this type of stylistic cleanup is something that can happen once the PR has landed.

deiga · 2025-12-09T07:14:57Z

github/data_source_github_users_test.go

+	if len(testAccConf.testExternalUser) == 0 {
+		t.Skip("No external user provided")
+	}


question: Would we benefit from this moving to acc_test.go as a skipUnlessExternal function?

It would do, once we've got the tests actually running in CI and the concepts are correct. I'm pretty sure the org app pattern in the tests at the moment is wrong and will need replacing, but all of this would be easier being incremental changes on top of a largely working test suite.

scripts/fmtcheck.sh

deiga · 2025-12-09T19:28:41Z

github/resource_github_organization_custom_properties.go

+		AllowedValues: allowedValuesString,
+	}
+
+	if val, ok := d.GetOk("values_editable_by"); ok {


question: Since this is a computed value, aren't there cases where ok would be false, but val has a value? Should we ignore the value in those cases?

This is in the resource create, we only want to send the value if it's been explicitly set. The computed part happenes when we set it after it's been created, at which point we can use that value. Without this change the implementation requires the value to be set as otherwise we're sending "" instead of nil.

deiga · 2025-12-09T19:29:47Z

github/resource_github_organization_project_test.go

question: Is there a reason to keep this file around? Are we planning to re-use it for projectsV2?

The other files exist, so this should be removed with them.

github/resource_github_organization_ruleset_test.go

deiga · 2025-12-09T19:33:31Z

github/resource_github_project_card_test.go

Same question as for project tests

deiga · 2025-12-09T19:33:45Z

github/resource_github_project_column_test.go

Same question as for project tests

deiga · 2025-12-09T19:47:14Z

github/resource_github_repository_collaborators_test.go

+							collaborators := state.RootModule().Resources["github_repository_collaborators.test_repo_collaborators"].Primary
+							for name, val := range collaborators.Attributes {
+								if strings.HasPrefix(name, "user.") && strings.HasSuffix(name, ".username") && val != testAccConf.testExternalUser {
+									return fmt.Errorf("expected user.*.username to be set to %s, was %s", testAccConf.testExternalUser, val)
+								}
+								if strings.HasPrefix(name, "user.") && strings.HasSuffix(name, ".permission") && val != "push" {
+									return fmt.Errorf("expected user.*.permission to be set to push, was %s", val)
+								}
+							}


question: Could this be replaced with a resource.TestCheckResourceAttr function call?
Or is there a purpose to this which I'm not seeing?

Maybe something like:

resource.TestCheckResourceAttr("github_repository_collaborators.test_repo_collaborators", "user.0.username", testAccConf.testExternalUser) resource.TestCheckResourceAttr("github_repository_collaborators.test_repo_collaborators", "user.0.permission", "push")

It's been a while since I wrote this, but I'm pretty sure that it solves the unknown ordering issue.

I get that, but there is only 1 collaborator in this config, right? If it's meant to be a helper for the sorting case, maybe it should be extracted for re-use?

Yeah it could be, but given the sheer amount of resources I've put into this PR I'd just like to get it merged and then it can be iterated on. I'd hate to lose something I spent time on that turns out later to be important. Once the code is merged anything removed can be retrieved, I think I've already lost work due to needing to get the tests passing.

Sure! Most of my suggestions are usually "address now, if capacity, otherwise do a follow-up".

I should learn to write that, since usually people can't rind my mind! 😬

That's fine. If this was a smaller piece of work or even if it was all fresh in my mind I'd be making more changes and being less cautious about removing code.

github/resource_github_repository_collaborators_test.go

Signed-off-by: Steve Hipwell <[email protected]>

deiga · 2025-12-10T14:34:18Z

github/resource_organization_block_test.go


-		blocked, _, err := conn.Organizations.IsBlocked(context.TODO(), orgName, username)
+		blocked, _, err := conn.Organizations.IsBlocked(context.Background(), orgName, username)


question: would we get access to t.Context() if we changed the function definition to func testAccCheckOrganizationBlockExists(ctx context.Context, n string)?

I cascaded t.Context() everywhere I could (other than for projects where I just got rid of context.TODO()), but some usage comes from typed functions being called directly. We could wrap up these function calls to pass it in, but IMHO this is a problem for another day.

Definitely, let's get this shippable!

stevehipwell requested a review from nickfloyd December 8, 2025 22:43

stevehipwell self-assigned this Dec 8, 2025

stevehipwell had a problem deploying to acctest-dotcom December 8, 2025 22:43 — with GitHub Actions Error

stevehipwell force-pushed the refactor-integration-tests branch from 64384d9 to a3a1145 Compare December 8, 2025 22:44

stevehipwell had a problem deploying to acctest-dotcom December 8, 2025 22:44 — with GitHub Actions Failure

stevehipwell had a problem deploying to acctest-dotcom December 8, 2025 22:44 — with GitHub Actions Error

stevehipwell force-pushed the refactor-integration-tests branch from a3a1145 to 79637e0 Compare December 8, 2025 22:46

stevehipwell had a problem deploying to acctest-dotcom December 8, 2025 22:46 — with GitHub Actions Error

stevehipwell force-pushed the refactor-integration-tests branch from 79637e0 to 8f8da76 Compare December 8, 2025 22:47

stevehipwell had a problem deploying to acctest-dotcom December 8, 2025 22:48 — with GitHub Actions Error

stevehipwell temporarily deployed to acctest-dotcom December 8, 2025 22:48 — with GitHub Actions Inactive

deiga reviewed Dec 9, 2025

View reviewed changes

stevehipwell force-pushed the refactor-integration-tests branch from 8f8da76 to d6872d3 Compare December 9, 2025 10:29

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 10:29 — with GitHub Actions Failure

stevehipwell temporarily deployed to acctest-dotcom December 9, 2025 10:29 — with GitHub Actions Inactive

stevehipwell force-pushed the refactor-integration-tests branch from d6872d3 to b273abd Compare December 9, 2025 12:29

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 12:30 — with GitHub Actions Failure

stevehipwell temporarily deployed to acctest-dotcom December 9, 2025 12:30 — with GitHub Actions Inactive

stevehipwell force-pushed the refactor-integration-tests branch from b273abd to 337b854 Compare December 9, 2025 14:28

stevehipwell added the acctest For CI / automated testing label Dec 9, 2025

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 14:28 — with GitHub Actions Error

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 17:26 — with GitHub Actions Error

stevehipwell temporarily deployed to acctest-dotcom December 9, 2025 17:26 — with GitHub Actions Inactive

stevehipwell force-pushed the refactor-integration-tests branch from be3c601 to 94d69ba Compare December 9, 2025 17:33

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 17:34 — with GitHub Actions Failure

stevehipwell temporarily deployed to acctest-dotcom December 9, 2025 17:34 — with GitHub Actions Inactive

stevehipwell had a problem deploying to acctest-dotcom December 9, 2025 17:34 — with GitHub Actions Failure

stevehipwell added this to the v6.9.1 Patch milestone Dec 9, 2025

deiga reviewed Dec 9, 2025

View reviewed changes

stevehipwell force-pushed the refactor-integration-tests branch from 94d69ba to 86c88d6 Compare December 10, 2025 10:40

stevehipwell had a problem deploying to acctest-dotcom December 10, 2025 10:40 — with GitHub Actions Error

stevehipwell force-pushed the refactor-integration-tests branch from 86c88d6 to e2e13b1 Compare December 10, 2025 11:19

stevehipwell had a problem deploying to acctest-dotcom December 10, 2025 11:19 — with GitHub Actions Error

stevehipwell force-pushed the refactor-integration-tests branch from e2e13b1 to f992480 Compare December 10, 2025 11:23

stevehipwell had a problem deploying to acctest-dotcom December 10, 2025 11:23 — with GitHub Actions Error

chore: Refactor integration tests

810b97c

Signed-off-by: Steve Hipwell <[email protected]>

stevehipwell force-pushed the refactor-integration-tests branch from f992480 to 810b97c Compare December 10, 2025 11:26

stevehipwell deployed to acctest-dotcom December 10, 2025 11:26 — with GitHub Actions Active

stevehipwell had a problem deploying to acctest-dotcom December 10, 2025 11:26 — with GitHub Actions Failure

deiga reviewed Dec 10, 2025

View reviewed changes

deiga mentioned this pull request Dec 10, 2025

[MAINT] Fix Org Ruleset tests #2976

Draft

5 tasks

stevehipwell modified the milestones: v6.9.1 Patch, v6.10.0 Release Dec 10, 2025

	depends_on = ["github_repository.test", "github_team.test", "github_team_repository.test"]
	depends_on = [github_team_repository.test]


		blocked, _, err := conn.Organizations.IsBlocked(context.TODO(), orgName, username)
		blocked, _, err := conn.Organizations.IsBlocked(context.Background(), orgName, username)

chore: Refactor integration tests #2986

Are you sure you want to change the base?

chore: Refactor integration tests #2986

Uh oh!

Conversation

stevehipwell commented Dec 8, 2025

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions bot commented Dec 8, 2025

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants