Skip to content

chore: Fail fast for migration DTO null contracts#1571

Merged
hatayama merged 1 commit into
v3-betafrom
chore/fail-fast-migration-dto-null-contracts
Jul 6, 2026
Merged

chore: Fail fast for migration DTO null contracts#1571
hatayama merged 1 commit into
v3-betafrom
chore/fail-fast-migration-dto-null-contracts

Conversation

@hatayama

@hatayama hatayama commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Summary

  • Align third-party tool migration DTOs with their existing non-null contracts.
  • Add pinning tests so invalid internal calls fail fast instead of being silently normalized.

User Impact

  • No supported migration workflow changes for valid projects.
  • Invalid internal migration data now surfaces immediately during development instead of being converted to empty values.

Changes

  • Replace migration rule data null fallbacks with ArgumentNullException after the existing Debug.Assert contracts.
  • Make migration file path snapshots reject null while keeping the empty-array fast path.
  • Add focused EditMode tests for the rule data and preview/result null contracts.

Verification

  • dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)" (0 errors, 0 warnings)
  • dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type exact --filter-value "io.github.hatayama.UnityCliLoop.Tests.Editor.ThirdPartyToolMigrationRulesTests" (164 passed)
  • dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type exact --filter-value "io.github.hatayama.UnityCliLoop.Tests.Editor.ThirdPartyToolMigrationFileServiceTests" (133 passed, 4 skipped)
  • dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value "(RemovedLegacyPlayerLoopTimingSignature_WhenOriginalParametersIsNull_Throws|RemovedLegacyPlayerLoopTimingSignature_WhenRemovedParametersIsNull_Throws|ThirdPartyToolMigrationContentResult_WhenContentIsNull_Throws|ThirdPartyToolMigrationContentResult_WhenRemovedSignaturesIsNull_Throws|ThirdPartyToolMigrationPreview_WhenFilePathsIsNull_Throws|ThirdPartyToolMigrationResult_WhenFilePathsIsNull_Throws)" (6 passed)

Review in cubic

Align migration DTO null handling with the existing Debug.Assert contracts so invalid internal calls fail fast instead of being silently normalized. Pin the contract behavior for rule data results and migration file path snapshots.
@coderabbitai

coderabbitai Bot commented Jul 6, 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: 2fd837c0-e975-4153-9ae7-996bb53d7b9a

📥 Commits

Reviewing files that changed from the base of the PR and between 071fd76 and c69749f.

📒 Files selected for processing (4)
  • Assets/Tests/Editor/ThirdPartyToolMigrationFileServiceTests.cs
  • Assets/Tests/Editor/ThirdPartyToolMigrationRulesTests.cs
  • Packages/src/Editor/Domain/ThirdPartyToolMigrationData.cs
  • Packages/src/Editor/Domain/ThirdPartyToolMigrationRuleData.cs

📝 Walkthrough

Walkthrough

Constructors and copy logic in third-party tool migration domain classes now throw ArgumentNullException for null arguments (filePaths, originalParameters, removedParameters, content, removedPlayerLoopTimingSignatures) instead of substituting empty defaults. Corresponding unit tests were added to verify this fail-fast behavior.

Changes

Fail-fast validation for migration domain objects

Layer / File(s) Summary
File path snapshot copy validation
Packages/src/Editor/Domain/ThirdPartyToolMigrationData.cs, Assets/Tests/Editor/ThirdPartyToolMigrationFileServiceTests.cs
Copy(string[] filePaths) now throws ArgumentNullException for null input rather than returning an empty array; empty arrays still return Array.Empty<string>(). New tests assert ThirdPartyToolMigrationPreview and ThirdPartyToolMigrationResult throw on null filePaths.
Rewrite rule constructor validation
Packages/src/Editor/Domain/ThirdPartyToolMigrationRuleData.cs, Assets/Tests/Editor/ThirdPartyToolMigrationRulesTests.cs
RemovedLegacyPlayerLoopTimingSignature and ThirdPartyToolMigrationContentResult constructors now throw ArgumentNullException for null originalParameters, removedParameters, content, and removedPlayerLoopTimingSignatures instead of defaulting to empty values. New tests added along with a using System; directive.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • hatayama/unity-cli-loop#1540: Prior PR that introduced the same fail-fast ArgumentNullException pattern in these same domain constructors and their tests.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: failing fast on null migration DTO contracts.
Description check ✅ Passed The description is directly related to the code changes and test updates in this pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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 chore/fail-fast-migration-dto-null-contracts

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.

@cubic-dev-ai cubic-dev-ai 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.

No issues found across 4 files

Re-trigger cubic

@hatayama
hatayama merged commit 89d225f into v3-beta Jul 6, 2026
10 checks passed
@hatayama
hatayama deleted the chore/fail-fast-migration-dto-null-contracts branch July 6, 2026 22:58
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.

1 participant