Skip to content

refactor(e2e): reuse scoped env helpers in dockercontext specs - #1648

Merged
skevetter merged 1 commit into
mainfrom
refactor/e2e-framework-target
Oct 22, 2026
Merged

skevetter merged 1 commit into
mainfrom
refactor/e2e-framework-target

Conversation

@skevetter

@skevetter skevetter commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • e2e/tests/dockercontext/dockercontext.go: both specs now use framework.UnsetEnvForSpec / framework.SetEnvForSpec for DOCKER_CONTEXT and DOCKER_HOST instead of hand-rolled LookupEnv/Setenv/Unsetenv and restore DeferCleanup blocks.
  • Spec names, docker-context label, assertions, and cleanup ordering unchanged.

Why

The two specs duplicated scoped env save/restore logic that framework.SetEnvForSpec/UnsetEnvForSpec already implement, including distinguishing unset from set-to-empty. Routing both specs through the helpers removes about 20 lines of restore code and keeps restore semantics on one tested path.

Validation

  • mise run cli:format: exit 0; diff touches only dockercontext.go
  • mise run cli:lint:ci: 0 issues
  • go vet ./e2e/...: pass
  • go test ./e2e/framework/...: pass
  • go tool ginkgo --dry-run --label-filter docker-context .: both docker-context specs selected
  • mise run cli:test:e2e:suite docker-context: 2 of 506 passed, 0 failed (local OrbStack Docker)

Note: task cli:format from the skill does not exist in this repo; mise run tasks were used instead.

Semantic change: helper errors on Set/Unset now fail via ExpectNoError; the old code discarded them with _ =.

CodeRabbit CLI

coderabbit review --agent --committed --base-commit c3952565: completed, 0 findings, 1 file reviewed.

Auto-merge

Eligible: yes, once CI is green on this head SHA. Non-test changed source lines: 0. Spec file: 24 changed lines (3 added, 21 removed), counted at half weight. Single file.

CI jobs that exercise docker-context: integration group, Linux, label: docker-context (hack/ci/suites.yaml:163).

Next

  • Confirm integration/docker-context is green on the head SHA before marking ready.

Summary by CodeRabbit

  • Tests
    • Docker context tests now run with explicitly controlled Docker context and host settings, reducing the chance that environment settings from elsewhere affect test results.
    • Environment setup and cleanup are handled consistently across the tests.

@skevetter skevetter added the e2e-refactor e2e refactor label Oct 22, 2026
@netlify

netlify Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for images-devsy-sh canceled.

Name Link
🔨 Latest commit 25b7e17
🔍 Latest deploy log https://app.netlify.com/projects/images-devsy-sh/deploys/6ad9b3848119083245106dc1

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0ecf4f09-5f7d-471c-bb3d-e0e334bda4a5
📥 Commits

Reviewing files that changed from the base of the PR and between c395256 and 25b7e17.

📒 Files selected for processing: 1
  • e2e/tests/dockercontext/dockercontext.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

📝 Walkthrough

The Docker context tests now use framework helpers to unset DOCKER_CONTEXT and DOCKER_HOST and to set DOCKER_CONTEXT. They no longer manually capture and restore these environment variables.

Changes: Docker context tests

Layer / File(s) Summary
Environment variable setup
e2e/tests/dockercontext/dockercontext.go
The tests use framework.UnsetEnvForSpec to unset DOCKER_CONTEXT and DOCKER_HOST, and framework.SetEnvForSpec to set DOCKER_CONTEXT. Manual environment capture and restoration were removed.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 25b7e

No actionable merge-blocking risk remains.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description explains the change, rationale, validation results, and semantic effect. However, it omits several required template sections, including the issue reference, type-of-change selection, … Update the description using the repository template. State the issue reference or write "none", select Refactor, complete the AI assistance and acknowledgement fields, document testing and manual verification, and complete the applicable c…
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main refactor: reusing scoped environment helpers in the Docker context specs.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

Full details: Description check

Explanation

The description explains the change, rationale, validation results, and semantic effect. However, it omits several required template sections, including the issue reference, type-of-change selection, AI assistance disclosure, checklist items, and the required testing/manual verification format.

Resolution

Update the description using the repository template. State the issue reference or write "none", select Refactor, complete the AI assistance and acknowledgement fields, document testing and manual verification, and complete the applicable checklist items.

✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@netlify

netlify Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for devsydev canceled.

Name Link
🔨 Latest commit 25b7e17
🔍 Latest deploy log https://app.netlify.com/projects/devsydev/deploys/6ad9b384c583f63d0a7a37a5

@skevetter
skevetter marked this pull request as ready for review October 22, 2026 08:45
@mergify

mergify Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

@skevetter
skevetter merged commit d1034e5 into main Oct 22, 2026
114 checks passed
@skevetter
skevetter deleted the refactor/e2e-framework-target branch October 22, 2026 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant