Skip to content

Fix orbit nudge test mdm connection fidelity#48423

Merged
getvictor merged 3 commits into
mainfrom
fix-orbit-nudge-test-mdm-connection-fidelity
Jun 30, 2026
Merged

Fix orbit nudge test mdm connection fidelity#48423
getvictor merged 3 commits into
mainfrom
fix-orbit-nudge-test-mdm-connection-fidelity

Conversation

@getvictor

@getvictor getvictor commented Jun 29, 2026

Copy link
Copy Markdown
Member

Related issue: Resolves #44629

Test fix only. Test now distinguishes between being connected to Fleet MDM and being connected but not osquery-enrolled.

[ ] QA'd all new/changed functionality manually

Summary by CodeRabbit

Summary by CodeRabbit

  • Tests
    • Improved validation of host configuration nudge behavior by refining how Fleet MDM connection states are simulated.
    • Test scenarios now better cover: enrolled but not connected to Fleet MDM, and connected to Fleet MDM without enrollment.

The non-eligible MDM status nudge test drove its two variations through
IsHostConnectedToFleetMDMFunc, but GetOrbitConfig now derives the Fleet-MDM
connection state from GetHostMDM().ConnectedToFleet and no longer calls
IsHostConnectedToFleetMDM. That left the isHostConnectedToFleet toggle dead:
both variations resolved to connected=false (GetHostMDM returned NotFound),
so the 'connected but not osquery-enrolled' variation no longer exercised the
osquery-enrollment gate.

Drive the connection state through GetHostMDM (ConnectedToFleet) so the second
variation is genuinely connected, restoring coverage of the not-osquery-enrolled
gate, and drop the now-dead IsHostConnectedToFleetMDMFunc mock.

Claude-Session: https://claude.ai/code/session_018vexp83A6iFZ6V53HSJAaR
@getvictor getvictor marked this pull request as ready for review June 29, 2026 16:45
@getvictor getvictor requested a review from a team as a code owner June 29, 2026 16:45
Copilot AI review requested due to automatic review settings June 29, 2026 16:45

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8e4afa5f-b69b-4dd9-95a1-9e1cb2513a76

📥 Commits

Reviewing files that changed from the base of the PR and between eb212ac and 18201d8.

📒 Files selected for processing (1)
  • server/service/orbit_test.go

Walkthrough

TestGetOrbitConfigNudge now simulates Fleet MDM connection state through ds.GetHostMDMFunc instead of a separate connection-check mock. The test uses a connectedToFleetMDM boolean to return a HostMDM with Enrolled: true and ConnectedToFleet: true when connected, and updates the host-variation cases to toggle that flag for the two scenarios.

Possibly related PRs

  • fleetdm/fleet#48375: Switches Orbit connectivity handling toward deriving Fleet MDM state from GetHostMDM, which matches the same test pattern change here.
🚥 Pre-merge checks | ✅ 1 | ❌ 4

❌ Failed checks (3 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR only adjusts a test and does not implement the query optimization or platform gating required by #44629. Update production code to reduce or eliminate the extra IsHostConnectedToFleetMDM JOIN as described in the linked issue.
Out of Scope Changes check ⚠️ Warning The only code change is a test rewrite, which is unrelated to the linked issue's production optimization work. Either implement the issue's runtime optimization or split the test-only adjustment into a separate PR.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description includes the related issue and a brief summary, but most template checklist sections are left unfilled. Add the applicable checklist items and clarify whether automated tests were updated and any QA was performed.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the test-only orbit MDM connection fidelity change.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-orbit-nudge-test-mdm-connection-fidelity

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Orbit service tests to reflect the recent change where GetOrbitConfig derives “connected to Fleet MDM” from GetHostMDM().ConnectedToFleet (rather than calling IsHostConnectedToFleetMDM). This ensures the nudge config tests differentiate between MDM connectivity and osquery enrollment state.

Changes:

  • Update TestGetOrbitConfigNudge to drive MDM connectivity via GetHostMDM mock data (including ConnectedToFleet).
  • Remove the IsHostConnectedToFleetMDM mock in the affected test section and adjust host-variation assertions accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/service/orbit_test.go
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.50%. Comparing base (624da7f) to head (18201d8).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #48423      +/-   ##
==========================================
+ Coverage   67.46%   67.50%   +0.03%     
==========================================
  Files        3675     3675              
  Lines      233402   233533     +131     
  Branches    12230    12230              
==========================================
+ Hits       157476   157635     +159     
+ Misses      61783    61748      -35     
- Partials    14143    14150       +7     
Flag Coverage Δ
backend 69.04% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: aggregate-result

Failed stage: Check for failures [❌]

Failed test name: vuln-mysql8.0.44

Failure summary:

The action failed because at least one test job reported a failure status in its downloaded status
artifact file.
- The vuln-mysql8.0.44 job’s status file (./vuln-mysql8.0.44-status/status) contained
fail, so the aggregation script added it to failed_tests.
- The workflow then exited with code 1
after printing ❌ One or more test jobs failed: vuln-mysql8.0.44 (lines 173-174, 191-192).

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

126:  Starting download of artifact to: /home/runner/work/fleet/fleet/scripts-status
127:  Extracting artifact entry: /home/runner/work/fleet/fleet/fast-status/status
128:  Extracting artifact entry: /home/runner/work/fleet/fleet/integration-core-mysql8.0.44-status/status
129:  Artifact download completed successfully.
130:  Artifact download completed successfully.
131:  Extracting artifact entry: /home/runner/work/fleet/fleet/main-mysql8.0.44-status/status
132:  Artifact download completed successfully.
133:  Extracting artifact entry: /home/runner/work/fleet/fleet/vuln-mysql8.0.44-status/status
134:  Artifact download completed successfully.
135:  Extracting artifact entry: /home/runner/work/fleet/fleet/service-mysql8.0.44-status/status
136:  Artifact download completed successfully.
137:  Extracting artifact entry: /home/runner/work/fleet/fleet/scripts-status/status
138:  Artifact download completed successfully.
139:  Total of 10 artifact(s) downloaded
140:  Download artifact has finished successfully
141:  ##[group]Run failed_tests=""
142:  �[36;1mfailed_tests=""�[0m
143:  �[36;1mstatus_count=0�[0m
144:  �[36;1m# Find all status files (they are in directories like 'fleetctl-mysql8.0.44-status/status')�[0m
145:  �[36;1mfor status_file in $(find ./ -type f -name 'status'); do�[0m
146:  �[36;1m  status_count=$((status_count + 1))�[0m
147:  �[36;1m  # Extract test name from parent directory (e.g., 'fleetctl-mysql8.0.44-status')�[0m
148:  �[36;1m  test_dir=$(basename $(dirname "$status_file"))�[0m
149:  �[36;1m  # Remove '-status' suffix to get the test name�[0m
150:  �[36;1m  test_name="${test_dir%-status}"�[0m
151:  �[36;1m  status_content=$(cat "$status_file")�[0m
152:  �[36;1m  echo "Processing: $status_file (Test: $test_name) with status content: $status_content"�[0m
153:  �[36;1m  if grep -q "fail" "$status_file"; then�[0m
154:  �[36;1m    echo "  ❌ Test failed: $test_name"�[0m
155:  �[36;1m    failed_tests="${failed_tests}${test_name}, "�[0m
156:  �[36;1m  else�[0m
157:  �[36;1m    echo "  ✅ Test passed: $test_name"�[0m
158:  �[36;1m  fi�[0m
159:  �[36;1mdone�[0m
160:  �[36;1mif [[ $status_count -eq 0 ]]; then�[0m
161:  �[36;1m  echo "❌ ERROR: No status files found! This indicates a workflow issue."�[0m
162:  �[36;1m  exit 1�[0m
163:  �[36;1mfi�[0m
164:  �[36;1mif [[ -n "$failed_tests" ]]; then�[0m
165:  �[36;1m  echo "❌ One or more test jobs failed: ${failed_tests%, }"�[0m
166:  �[36;1m  exit 1�[0m
167:  �[36;1mfi�[0m
168:  �[36;1mecho "✅ All test jobs succeeded."�[0m
169:  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
170:  ##[endgroup]
171:  Processing: ./fleetctl-mysql8.0.44-status/status (Test: fleetctl-mysql8.0.44) with status content: success
172:  ✅ Test passed: fleetctl-mysql8.0.44
173:  Processing: ./vuln-mysql8.0.44-status/status (Test: vuln-mysql8.0.44) with status content: fail
174:  ❌ Test failed: vuln-mysql8.0.44
175:  Processing: ./service-mysql8.0.44-status/status (Test: service-mysql8.0.44) with status content: success
176:  ✅ Test passed: service-mysql8.0.44
177:  Processing: ./integration-core-mysql8.0.44-status/status (Test: integration-core-mysql8.0.44) with status content: success
178:  ✅ Test passed: integration-core-mysql8.0.44
179:  Processing: ./mysql-mysql8.0.44-status/status (Test: mysql-mysql8.0.44) with status content: success
180:  ✅ Test passed: mysql-mysql8.0.44
181:  Processing: ./integration-enterprise-mysql8.0.44-status/status (Test: integration-enterprise-mysql8.0.44) with status content: success
182:  ✅ Test passed: integration-enterprise-mysql8.0.44
183:  Processing: ./integration-mdm-mysql8.0.44-status/status (Test: integration-mdm-mysql8.0.44) with status content: success
184:  ✅ Test passed: integration-mdm-mysql8.0.44
185:  Processing: ./scripts-status/status (Test: scripts) with status content: success
186:  ✅ Test passed: scripts
187:  Processing: ./fast-status/status (Test: fast) with status content: success
188:  ✅ Test passed: fast
189:  Processing: ./main-mysql8.0.44-status/status (Test: main-mysql8.0.44) with status content: success
190:  ✅ Test passed: main-mysql8.0.44
191:  ❌ One or more test jobs failed: vuln-mysql8.0.44
192:  ##[error]Process completed with exit code 1.
193:  Post job cleanup.

@getvictor getvictor merged commit 5d531c1 into main Jun 30, 2026
43 of 45 checks passed
@getvictor getvictor deleted the fix-orbit-nudge-test-mdm-connection-fidelity branch June 30, 2026 20:32
Leanngove pushed a commit that referenced this pull request Jul 2, 2026
<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**Related issue:** Resolves #44629

Test fix only. Test now distinguishes between being connected to Fleet
MDM and being connected but not osquery-enrolled.

[ ] QA'd all new/changed functionality manually

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

* **Tests**
* Improved validation of host configuration nudge behavior by refining
how Fleet MDM connection states are simulated.
* Test scenarios now better cover: enrolled but not connected to Fleet
MDM, and connected to Fleet MDM without enrollment.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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.

[top steady-state query] Optimize IsHostConnectedToFleetMDM 3-table JOIN on every orbit check-in for every host

3 participants