Skip to content

Conversation

@pietern
Copy link
Contributor

@pietern pietern commented Dec 16, 2024

Changes

The Setenv helper function configures an environment variable and resets it to its original value when exiting the test scope. It is incompatible with running tests in parallel because it modifies process-wide state. The libs/env package defines functions to interact with the environment but records Setenv calls on a context.Context. This enables us to override/specialize the environment scoped to a context.

Pre-requisites for removing the t.Setenv calls:

  • Make cmdio.NewIO accept a context and use it with libs/env
  • Make all internal/testcli functions use a context

The rest of this change:

  • Modifies integration tests to initialize a context to use if there wasn't already one
  • Updates t.Setenv calls to use env.Set

Tests

n/a

@pietern
Copy link
Contributor Author

pietern commented Dec 16, 2024

It would be nicer to have a linter to prevent t.Setenv calls from the integration tests but I couldn't find one.

@github-actions
Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2018
  • Commit SHA: c2f8952055be140356d09814444a9c96f54596b4

Checks will be approved automatically on success.

@pietern
Copy link
Contributor Author

pietern commented Dec 16, 2024

The integration test TestFetchRepositoryInfoAPI failed because the cleanup reused the context.

The context already had a call to dbr.MockRuntime and the command execution calls dbr.DetectRuntime.

Use the fixture helpers instead of calling commands to fix this.

Copy link
Contributor

@denik denik left a comment

Choose a reason for hiding this comment

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

Regarding "panic: dbr.DetectRuntime called twice on the same context" and changes in git_fetch_test.go.

While it does look cleaner to use helpers, it's weird that you can longer call "workspace mkdirs" followed by "workspace delete" - this suggests that something else is broken.

Another thing that is not clear is why does DetectRuntime fails when called second time instead of being idempotent?

}

func ensureWorkspacePrefix(root string) string {
// The fixture helper doesn't include /Workspace, so include it here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fix the fixture helper instead to generate right path?

Copy link
Contributor Author

@pietern pietern Dec 16, 2024

Choose a reason for hiding this comment

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

I tried this but there are assumptions elsewhere on the prefix not being included.

I'd like to address this in a separate PR and normalize on having the /Workspace prefix everywhere.

@pietern
Copy link
Contributor Author

pietern commented Dec 16, 2024

While it does look cleaner to use helpers, it's weird that you can longer call "workspace mkdirs" followed by "workspace delete" - this suggests that something else is broken.

This was prevented by the dbr.MockRuntime call sandwiched in between these calls.

It panics on the second call as a sanity check that we're not mocking/detecting multiple times.

@pietern
Copy link
Contributor Author

pietern commented Dec 16, 2024

The integration tests passed. Only the hooks to make a comment and mark the check as successful failed.

@pietern pietern merged commit 70b7bbf into main Dec 16, 2024
10 checks passed
@pietern pietern deleted the integration-no-setenv branch December 16, 2024 11:34
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.

3 participants