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.

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!

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

deiga · 2025-12-24T23:06:35Z

CONTRIBUTING.md

-	]
+  "go.testEnvVars": {
+    "TF_ACC": "1",
+    "GITHUB_TOKEN": "<TOKEN>",


Suggested change

"GITHUB_TOKEN": "<TOKEN>",

"GH_TOKEN": "<TOKEN>",

deiga · 2025-12-24T23:14:52Z

github/resource_github_repository_collaborator_test.go

+		// Note: This test requires GH_TEST_COLLABORATOR to be set to a valid GitHub username
+		testCollaborator := os.Getenv("GH_TEST_COLLABORATOR")
 		if testCollaborator == "" {
-			t.Skip("GITHUB_TEST_COLLABORATOR not set, skipping archived repository collaborator test")
+			t.Skip("GH_TEST_COLLABORATOR not set, skipping archived repository collaborator test")
 		}


question: Should this use testAccConf.testExternalUser as well?

deiga · 2025-12-24T23:24:12Z

github/acc_test.go

+		config.owner = os.Getenv("GITHUB_OWNER")
+		config.username = os.Getenv("GITHUB_USERNAME")
+		config.token = os.Getenv("GITHUB_TOKEN")


question: The CONTRIBUTING.md talks about GH_OWNER, GH_USERNAME and GH_TOKEN which one is it?

deiga · 2025-12-24T23:29:25Z

GNUmakefile

-testacc: fmtcheck
-	TF_ACC=1 CGO_ENABLED=0 go test $(TEST) -v $(TESTARGS) -timeout 120m
+testacc:
+	TF_ACC=1 CGO_ENABLED=0 go test -run "^TestAcc*" $(TEST) -v $(TESTARGS) -timeout 120m -count=1


question: With this format there is no way to run only a specific subset of acctests, is that intentional?

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 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

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

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

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

deiga mentioned this pull request Dec 17, 2025

[BUG]: Acceptance Tests Failing #3006

Open

1 task

stevehipwell force-pushed the refactor-integration-tests branch from 810b97c to 14b301a Compare December 19, 2025 16:31

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

stevehipwell had a problem deploying to acctest-dotcom December 19, 2025 16:31 — with GitHub Actions Failure

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

chore: Refactor integration tests

450176c

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell force-pushed the refactor-integration-tests branch from 14b301a to 450176c Compare December 19, 2025 16:56

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

stevehipwell had a problem deploying to acctest-dotcom December 19, 2025 16:56 — with GitHub Actions Failure

stevehipwell deployed to acctest-dotcom December 19, 2025 16:56 — with GitHub Actions Active

deiga reviewed Dec 24, 2025

View reviewed changes

	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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants