also looking at this
fix(rules): apply a rule's autoMigrate, closing the rule-effects family
#1423
2 files
+90
-1
| @@ -3,6 +3,7 @@ import type { BuddyConfig, PackageUpdate } from '../types' | ||
| 3 | 3 | import type { Logger } from '../utils/logger' |
| 4 | 4 | import type { UpgradeResult } from './migrate' |
| 5 | 5 | import { stripManifest } from '../pr/pr-manifest' |
| 6 | import { groupsToRules, resolveRuleEffects } from '../rules/engine' | |
| 6 | 7 | import { getDefaultLogger } from '../utils/logger' |
| 7 | 8 | import { attemptMajorUpgrade } from './migrate' |
| 8 | 9 | import { collectSpanNotes, describeSpanGaps } from './span' |
| @@ -92,6 +93,10 @@ export async function analyzeGroupMajors(options: GroupUpgradeOptions): Promise< | ||
| 92 | 93 | const results: GroupUpgradeOutcome['results'] = [] |
| 93 | 94 | const skipped: GroupUpgradeOutcome['skipped'] = [] |
| 94 | 95 | |
| 96 | // `autoMigrate` on a package rule was resolved into the effects and read by | |
| 97 | // nobody, so only the repository-wide setting ever reached the migration. | |
| 98 | const rules = [...groupsToRules(options.config.packages?.groups), ...(options.config.packages?.rules ?? [])] | |
| 99 | ||
| 95 | 100 | for (const update of majors) { |
| 96 | 101 | if (!matchesGlobs(update.name, settings.packages)) { |
| 97 | 102 | skipped.push({ name: update.name, reason: 'not matched by ai.majorUpgrades.packages' }) |
| @@ -103,6 +108,14 @@ export async function analyzeGroupMajors(options: GroupUpgradeOptions): Promise< | ||
| 103 | 108 | const span = collectSpanNotes(releases, update.currentVersion, update.newVersion) |
| 104 | 109 | const gaps = describeSpanGaps(span) |
| 105 | 110 | |
| 111 | // Resolved per package rather than per group: this loop already runs | |
| 112 | // once per major, and the documented reading — "one package opting in is | |
| 113 | // enough, since migrating what can be migrated leaves everything else | |
| 114 | // exactly as it would have been" — is an argument for deciding each on | |
| 115 | // its own rather than letting one opt-in carry the rest. | |
| 116 | const ruled = rules.length > 0 ? resolveRuleEffects(update, rules).autoMigrate : undefined | |
| 117 | const autoMigrate = ruled ?? settings.autoMigrate | |
| 118 | ||
| 106 | 119 | const result = await attemptMajorUpgrade({ |
| 107 | 120 | update, |
| 108 | 121 | releaseNotes: span.notes, |
| @@ -110,7 +123,7 @@ export async function analyzeGroupMajors(options: GroupUpgradeOptions): Promise< | ||
| 110 | 123 | workspace: options.workspace, |
| 111 | 124 | baseBranch: options.baseBranch, |
| 112 | 125 | ai: options.ai, |
| 113 | ...(settings.autoMigrate !== undefined ? { autoMigrate: settings.autoMigrate } : {}), | |
| 126 | ...(autoMigrate !== undefined ? { autoMigrate } : {}), | |
| 114 | 127 | ...(settings.draftBelowConfidence ? { draftBelowConfidence: settings.draftBelowConfidence } : {}), |
| 115 | 128 | logger, |
| 116 | 129 | }) |
| @@ -220,3 +220,79 @@ describe('report placement', () => { | ||
| 220 | 220 | expect(appendUpgradeReport('## Tables', 'report')).toContain('report') |
| 221 | 221 | }) |
| 222 | 222 | }) |
| 223 | ||
| 224 | /** | |
| 225 | * `autoMigrate` on a package rule was resolved into `ResolvedEffects` and | |
| 226 | * aggregated by `mergeGroupEffects`, and nothing read either — only the | |
| 227 | * repository-wide `ai.majorUpgrades.autoMigrate` ever reached the migration. | |
| 228 | */ | |
| 229 | describe('rule-level autoMigrate', () => { | |
| 230 | /** A plan with a change, so attempting the migration is distinguishable. */ | |
| 231 | const WITH_CHANGES = { | |
| 232 | changes: [{ | |
| 233 | description: 'the legacy import was removed', | |
| 234 | action: 'import from the package root instead', | |
| 235 | affectedFiles: [], | |
| 236 | automatable: true, | |
| 237 | }], | |
| 238 | confidence: 'high', | |
| 239 | effort: 2, | |
| 240 | risks: [], | |
| 241 | } | |
| 242 | ||
| 243 | function configWith(rules: unknown[], globalAutoMigrate?: boolean): BuddyConfig { | |
| 244 | return { | |
| 245 | ai: { | |
| 246 | majorUpgrades: { | |
| 247 | enabled: true, | |
| 248 | ...(globalAutoMigrate === undefined ? {} : { autoMigrate: globalAutoMigrate }), | |
| 249 | }, | |
| 250 | }, | |
| 251 | packages: { strategy: 'all', rules }, | |
| 252 | } as unknown as BuddyConfig | |
| 253 | } | |
| 254 | ||
| 255 | /** Analysis alone is one completion; attempting the migration adds another. */ | |
| 256 | async function completions(config: BuddyConfig, update = makeUpdate()): Promise<number> { | |
| 257 | const ai = scriptedClient(WITH_CHANGES) | |
| 258 | await analyzeGroupMajors({ ...baseOptions(config, ai), updates: [update] }) | |
| 259 | return ai.requests.length | |
| 260 | } | |
| 261 | ||
| 262 | it('success case - a rule turns the migration on where the global is off', async () => { | |
| 263 | expect(await completions(configWith([{ matchPackages: ['react'], autoMigrate: true }]))).toBe(2) | |
| 264 | }) | |
| 265 | ||
| 266 | it('failure case - a rule turns it off where the global is on', async () => { | |
| 267 | expect(await completions(configWith([{ matchPackages: ['react'], autoMigrate: false }], true))).toBe(1) | |
| 268 | }) | |
| 269 | ||
| 270 | it('success case - the global still applies where no rule matches', async () => { | |
| 271 | expect(await completions(configWith([{ matchPackages: ['lodash'], autoMigrate: true }], true))).toBe(2) | |
| 272 | expect(await completions(configWith([{ matchPackages: ['lodash'], autoMigrate: true }], false))).toBe(1) | |
| 273 | }) | |
| 274 | ||
| 275 | it('success case - each package decides for itself', async () => { | |
| 276 | // The documented reading is that migrating what can be migrated leaves | |
| 277 | // everything else exactly as it would have been — which is an argument | |
| 278 | // for resolving per package rather than letting one opt-in carry a group. | |
| 279 | const ai = scriptedClient(WITH_CHANGES) | |
| 280 | await analyzeGroupMajors({ | |
| 281 | ...baseOptions(configWith([{ matchPackages: ['react'], autoMigrate: true }]), ai), | |
| 282 | updates: [makeUpdate({ name: 'react' }), makeUpdate({ name: 'lodash' })], | |
| 283 | }) | |
| 284 | ||
| 285 | // Two analyses, and one migration attempt — react's. | |
| 286 | expect(ai.requests.length).toBe(3) | |
| 287 | }) | |
| 288 | ||
| 289 | it('edge case - a rule saying nothing about migration changes nothing', async () => { | |
| 290 | expect(await completions(configWith([{ matchPackages: ['react'], groupName: 'React' }], false))).toBe(1) | |
| 291 | expect(await completions(configWith([{ matchPackages: ['react'], groupName: 'React' }], true))).toBe(2) | |
| 292 | }) | |
| 293 | ||
| 294 | it('edge case - no rules leaves the global in charge', async () => { | |
| 295 | expect(await completions(configWith([], true))).toBe(2) | |
| 296 | expect(await completions(configWith([]))).toBe(1) | |
| 297 | }) | |
| 298 | }) | |