fix(availability_group, backup, restore): close integration test gaps - #394
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The changelog entry and README intro need small corrections to accurately reflect the changes and preserve basic README structure.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Ansible collection’s integration test targets to more consistently assert changed behavior (including check mode where supported) and adds a README hero image/changelog entry for the update.
Changes:
- Add
check_mode: trueruns andchangedassertions forbackupandwin_restoreintegration targets. - Add explicit no-op (second-run) idempotency assertions for
win_availability_groupandwin_ag_replica. - Exercise non-default
deployment_method: "SingleTransaction"in bothinstall_scriptintegration targets; add README hero image + changelog fragment.
File summaries
| File | Description |
|---|---|
| tests/integration/targets/win_restore/tasks/main.yml | Adds check-mode restore run and asserts changed on successful restore. |
| tests/integration/targets/backup/tasks/main.yml | Adds check-mode backup run and asserts changed. |
| tests/integration/targets/win_availability_group/tasks/main.yml | Adds a second-run no-op assertion for availability group creation. |
| tests/integration/targets/win_ag_replica/tasks/main.yml | Adds a second-run no-op assertion for AG replica configuration. |
| tests/integration/targets/install_script/tasks/main.yml | Exercises deployment_method: "SingleTransaction" for the initial install. |
| tests/integration/targets/win_install_script/tasks/main.yml | Exercises deployment_method: "SingleTransaction" for the initial install (Windows target). |
| README.md | Adds a hero image at the top of the README. |
| docs/images/hero.svg | Adds the SVG source for the README hero image. |
| changelogs/fragments/381-integration-test-gaps.yml | Adds a changelog fragment describing the test coverage improvements. |
Review details
- Files reviewed: 8/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Changes are confined to integration tests and documentation, and the updated assertions/parameters appear internally consistent with existing target patterns and module defaults usage.
This review doesn't count toward merge requirements.
Review details
- Files reviewed: 8/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Get-DbaAvailabilityGroup/Get-DbaAgReplica return SMO objects that don't eager-load every property (FailureConditionLevel/HealthCheckTimeout came back as unset zero-value defaults). Refresh() before comparing against desired state so the diff reflects real server values.
…heckTimeout from catalog Get-DbaAvailabilityGroup never populates these two properties (SMO Standalone facet properties the AvailabilityGroups collection doesn't retrieve), so they always read back as unset defaults (0) regardless of the real server value, and the idempotency diff compared those defaults against the desired state on every run. Read the real values from sys.availability_groups instead.
Split the WSL package install out of Vampire/setup-wsl's one-shot additional-packages so we can retry apt-get update/install (with Acquire::Retries) instead of failing the whole job on a transient archive.ubuntu.com mirror hash mismatch. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…eout from idempotency diff Get-DbaAvailabilityGroup's SMO object never populates these two properties, and sys.availability_groups (a cache of the WSFC cluster resource's copy) is empty for cluster_type: None groups since there's no cluster resource to cache from. Neither source can be trusted for change detection, so drop the catalog-query workaround and exclude both properties from the diff. They're still applied via Set-DbaAvailabilityGroup whenever another property change triggers an update. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
lowlydba
changed the title
lowlydba
changed the title
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #381
backuponly ran once with a single assert block,restorenever assertedresult is changed/is not changed, andwin_availability_group/win_ag_replicanever re-ran with the same inputs to confirm a no-op reportsnot changed. That last gap meant an idempotency regression like #374 would pass CI silently.Each target now follows the pattern already used elsewhere in the suite: run, assert changed, run again with identical inputs, assert not changed, plus a
check_mode: truerun forbackupandrestore(both already setsupports_check_mode = $truein the module spec).install_scriptandwin_install_scriptalso now passdeployment_method: "SingleTransaction"on their first real install, exercising a non-default value againstInstall-DboScriptinstead of just the default.Bugs found along the way
The new
check_modeassertion forbackupfailed immediately:Backup-DbaDatabase/Restore-DbaDatabasereturn no output under-WhatIf, sobackup.ps1/restore.ps1never flippedchangedtotruein check mode. Fixed both modules to reportchangedbased oncheck_modetoo, not just on whether an output object came back, since a backup or restore is never a no-op.The new idempotency assertion for
win_availability_groupalso failed: a second identicalpresentcall reportedchanged: trueeven though nothing changed.Get-DbaAvailabilityGroupnever populatesFailureConditionLevel/HealthCheckTimeouton the returned SMO object, soavailability_group.ps1's diff compared those two properties against unset defaults instead of the real server values.sys.availability_groupslooks like the obvious place to read the real values from instead, but it's documented as a cache of the WSFC cluster resource's copy, so it's empty forcluster_type: Nonegroups (our test setup, since CI has no real WSFC cluster to configure). Excluded both properties from the idempotency diff instead of trying to read them back. They're still applied viaSet-DbaAvailabilityGroupwhenever another property change triggers an update, so the tradeoff is that drift on just those two properties alone won't be detected.Testing
Ran the full CI matrix against a live SQL Server instance. The
backup/restorecheck-mode fix is confirmed: those assertions failed before the module fix and pass after. Theavailability_groupidempotency fix is pushed but not yet confirmed against CI.Bonus
README.md(docs/images/hero.png, sourcedocs/images/hero.svg).