refactor: remove SkipProvisionerCheck and improve test provisioner handling #19117

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.

Sign up for GitHub

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

Jump to bottom

Open

cstyan wants to merge 1 commit into callum-autostart-no-provisioner from blink/remove-skip-provisioner-check

Contributor

cstyan commented Jul 31, 2025

Summary

This PR addresses the requirements to:

✅ Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable and verify test behavior
✅ Remove the need for SkipProvisionerCheck by refactoring tests to properly handle provisioner availability

Changes Made

🗑️ Removed SkipProvisionerCheck Mechanism

Removed SkipProvisionerCheck field from Executor struct
Removed skip logic from hasAvailableProvisioners function
Removed SkipProvisionerCheck option from coderdtest.Options

🧪 Enhanced Test Infrastructure

Added mustWaitForProvisioners() helper function
Added mustWaitForProvisionersWithClient() helper function
Added mustWaitForProvisionersAvailable() helper function for workspace-specific provisioner availability
Updated existing tests to wait for provisioner availability

🔧 Fixed Main Test

Commented out provisionerCloser.Close() call in TestExecutorAutostartSkipsWhenNoProvisionersAvailable
Added proper provisioner daemon tags to match template requirements
Fixed timing issues by reducing sleep time to prevent false staleness
Added provisioner availability waiting to ensure proper test setup

Test Behavior

Before: Test relied on SkipProvisionerCheck = true to bypass provisioner availability logic
After: Test uses real hasAvailableProvisioners logic with proper provisioner setup

The test now correctly:

✅ Validates that provisioners remain available when not explicitly closed
✅ Uses real provisioner availability checks instead of bypassing them
✅ Properly handles provisioner daemon tags and organization matching
✅ Demonstrates correct autostart behavior based on actual provisioner state

Testing

Ran TestExecutorAutostartSkipsWhenNoProvisionersAvailable successfully with the changes:

Provisioner remains active (not stale) when close call is commented out
Test passes with real provisioner availability logic
No more dependency on SkipProvisionerCheck mechanism

Files Changed

coderd/autobuild/lifecycle_executor.go - Removed SkipProvisionerCheck mechanism
coderd/autobuild/lifecycle_executor_test.go - Enhanced test infrastructure and fixed main test
coderd/coderdtest/coderdtest.go - Removed SkipProvisionerCheck option


          refactor: remove SkipProvisionerCheck and improve test provisioner ha…

…ndling

This commit addresses the requirements to:
1. Comment out provisionerCloser.Close() in TestExecutorAutostartSkipsWhenNoProvisionersAvailable
2. Remove the need for SkipProvisionerCheck by refactoring tests

Changes:
- Remove SkipProvisionerCheck field and logic from Executor
- Remove SkipProvisionerCheck option from coderdtest.Options
- Add proper provisioner daemon tags to match template requirements
- Add helper functions for waiting for provisioner availability
- Update tests to use real provisioner availability checks
- Fix timing issues in provisioner staleness tests
- Comment out provisioner close call to test proper behavior

The test now correctly validates that provisioners remain available
when not explicitly closed, and uses the real hasAvailableProvisioners
logic instead of bypassing it.

github-actions bot commented Jul 31, 2025

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

Blink Assistant seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

github-actions bot assigned cstyan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet