Add migration system coverage tests (550+ tests)#997
Add migration system coverage tests (550+ tests)#997Godzilla675 wants to merge 14 commits intozio:mainfrom
Conversation
…structure comparison - Add validatePath/validatePathRecursive to check Wrap path segments match schema structure - Validate Elements only on sequences, MapKeys/MapValues only on maps, Case only on variants - Add lenient structure comparison mode when structural migration actions are present - Handle Split/Join/Mandate/Optionalize by relaxing field presence checks - Preserve optionality checking for Dynamic fields even in lenient mode - Fix error message format for optionality and structure mismatches Fixes 13 failing MigrationValidatorSpec tests and 1 MigrationValidatorOptionalitySpec test.
Add 70+ tests targeting specific uncovered branches: - DynamicSchemaExpr: Path/DefaultValue/ResolvedDefault/Literal eval - DynamicSchemaExpr: coercePrimitive identity + cross-type conversions - DynamicSchemaExpr: evalPrimitiveArithmetic all type combos - MigrationValidator: compareStructures all match arms - MigrationValidator: validatePathRecursive all node error paths - MigrationValidator: Join/Split path-must-end-in-field errors
There was a problem hiding this comment.
Pull request overview
Adds a new schema migration subsystem to zio.blocks.schema (typed Migration wrapping a serializable DynamicMigration + MigrationAction + DynamicSchemaExpr), plus extensive test coverage and documentation updates to meet branch coverage targets.
Changes:
- Introduces the migration core APIs (
Migration,DynamicMigration,MigrationAction,DynamicSchemaExpr,MigrationBuilder,Selector) and Scala 2/3 selector-macro syntax. - Adds ~550 tests across migration components, plus Scala 3-only structural-schema tests.
- Updates docs/examples and test helpers (newline normalization), and adjusts scalafmt dialect overrides.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/shared/src/test/scala/zio/blocks/schema/migration/SelectorMacrosSpec.scala | Tests Scala selector-macro path extraction to DynamicOptic. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/SelectorCoverageSpec.scala | Coverage tests for Selector composition and toOptic. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationValidatorOptionalitySpec.scala | Regression test for optionality mismatch validation. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationValidatorCoverageSpec.scala | Broad branch coverage for MigrationValidator logic. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationSpec.scala | Tests typed Migration behavior (apply/applyDynamic/composition/reverse). |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationSchemasSerializationSpec.scala | Validates JSON codec round-trip for DynamicMigration. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationSchemasCoverageSpec.scala | Round-trip tests for schemas of migration ADTs. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationErrorSpec.scala | Tests MigrationError messages and composition. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationErrorCoverageSpec.scala | Coverage tests for all MigrationError variants. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationCoverageSpec.scala | Coverage tests for Migration helpers and composition. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationBuilderSpec.scala | High-level usage tests for MigrationBuilder. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationBuilderCoverageSpec.scala | Coverage tests for MigrationBuilder API + helpers. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationActionSpec.scala | Tests action reversibility semantics. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/MigrationActionCoverageSpec.scala | Coverage tests for MigrationAction.reverse across variants. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/DynamicSchemaExprSpec.scala | Tests DynamicSchemaExpr.eval for many operators. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/DynamicSchemaExprSerializationSpec.scala | Serialization round-trip tests for DynamicSchemaExpr. |
| schema/shared/src/test/scala/zio/blocks/schema/migration/DefaultValueResolutionSpec.scala | Tests build-time DefaultValue resolution behavior. |
| schema/shared/src/test/scala/zio/blocks/schema/json/JsonTestUtils.scala | Normalizes newlines for JSON golden-string assertions. |
| schema/shared/src/test/scala-3/zio/blocks/schema/migration/StructuralMigrationApplySpec.scala | Scala 3-only tests for structural schemas and typed apply erroring. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/package.scala | Adds migration package object scaladoc entry point. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/Selector.scala | Adds Selector abstraction for typed path composition. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/MigrationError.scala | Adds migration error model with path-aware messages. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/MigrationBuilder.scala | Adds fluent builder + build validation + default resolution. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/MigrationAction.scala | Defines migration action algebra + reverse. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/Migration.scala | Adds typed migration wrapper + composition + reverse. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/DynamicSchemaExpr.scala | Adds serializable expression language for transforms. |
| schema/shared/src/main/scala/zio/blocks/schema/migration/DynamicMigration.scala | Adds action interpreter over DynamicValue. |
| schema/shared/src/main/scala/zio/blocks/schema/json/JsonBinaryCodecDeriver.scala | Fixes delayed usedRegisters init for recursive records in codecs. |
| schema/shared/src/main/scala/zio/blocks/schema/binding/Matcher.scala | Adds StructuralMatcher for structural types. |
| schema/shared/src/main/scala/zio/blocks/schema/binding/Deconstructor.scala | Adds StructuralDeconstructor marker/abstraction. |
| schema/shared/src/main/scala/zio/blocks/schema/binding/Constructor.scala | Adds StructuralConstructor marker/abstraction. |
| schema/shared/src/main/scala-3/zio/blocks/schema/migration/SelectorMacros.scala | Scala 3 macro implementation for selector lambdas to DynamicOptic. |
| schema/shared/src/main/scala-3/zio/blocks/schema/migration/MigrationBuilderSyntax.scala | Scala 3 inline extension syntax for selector-based builder ops. |
| schema/shared/src/main/scala-3/zio/blocks/schema/SchemaCompanionVersionSpecific.scala | Adds Scala 3 Schema.structural derivation for refinement/union types. |
| schema/shared/src/main/scala-2/zio/blocks/schema/migration/SelectorMacros.scala | Scala 2 macro implementation for selector lambdas to DynamicOptic. |
| schema/shared/src/main/scala-2/zio/blocks/schema/migration/MigrationBuilderSyntax.scala | Scala 2 macro-based syntax for selector-based builder ops. |
| schema/shared/src/main/scala-2/zio/blocks/schema/SchemaCompanionVersionSpecific.scala | Adds Scala 2 stub macro for Schema.structural (Scala 3-only feature). |
| schema-toon/src/test/scala/zio/blocks/schema/toon/ToonTestUtils.scala | Normalizes newlines for Toon golden-string assertions. |
| docs/reference/validation.md | Updates examples to use Chunk instead of Vector for DynamicValue constructors. |
| docs/reference/schema.md | Updates DynamicValue examples to use Chunk. |
| docs/reference/migration.md | Adds migration system reference documentation. |
| .scalafmt.conf | Overrides scala213 dialect for scala-2 sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```scala | ||
| Migration | ||
| .newBuilder[OldType, NewType] | ||
| // Record operations | ||
| .addField(path, defaultExpr) | ||
| .dropField(path, defaultForReverse) | ||
| .renameField(fromPath, toPath) | ||
| .transformField(path, transform, reverseTransform) | ||
| .mandateField(path, default) | ||
| .optionalizeField(path) | ||
| .changeType(path, converter, reverseConverter) | ||
| .joinFields(targetPath, sourcePaths, combiner, splitter) | ||
| .splitField(sourcePath, targetPaths, splitter, combiner) | ||
| // Enum operations | ||
| .renameCase(path, from, to) | ||
| .transformCase(path, caseName, nestedActions) | ||
| // Collection operations | ||
| .transformElements(path, transform, reverseTransform) | ||
| .transformKeys(path, transform, reverseTransform) | ||
| .transformValues(path, transform, reverseTransform) |
There was a problem hiding this comment.
The MigrationBuilder API snippet lists methods that don't match the actual builder surface (e.g. .changeType vs MigrationBuilder.changeFieldType, and .renameCase(path, ...) / .transformCase(path, ...) vs renameCaseAt / transformCaseAt). Updating these names in the docs will prevent copy/paste examples from failing to compile.
| modifyAtPathWithParentContext(value, at) { (_, targetValue) => | ||
| transform.eval(targetValue).left.map(wrapExprError(at, "TransformValue")) |
There was a problem hiding this comment.
TransformValue evaluates the expression on the field value (targetValue). That makes common transforms like Arithmetic(Path(root.field("age")), ...) fail because Path expects a record/parent context (similar to how AddField/Mandate/ChangeType evaluate against the surrounding context). Consider evaluating transform against the parent context from modifyAtPathWithParentContext (and then writing the resulting value back at at).
| modifyAtPathWithParentContext(value, at) { (_, targetValue) => | |
| transform.eval(targetValue).left.map(wrapExprError(at, "TransformValue")) | |
| modifyAtPathWithParentContext(value, at) { (context, _) => | |
| transform.eval(context).left.map(wrapExprError(at, "TransformValue")) |
| case MigrationAction.Mandate(at, default) => | ||
| modifyAtPathWithParentContext(value, at) { (context, targetValue) => | ||
| targetValue match { | ||
| case DynamicValue.Variant("None", _) => | ||
| default.eval(context).left.map(wrapExprError(at, "Mandate")) | ||
| case DynamicValue.Variant("Some", inner) => | ||
| Right(inner) | ||
| case other => | ||
| // Already a non-optional value, return as-is | ||
| Right(other) | ||
| } | ||
| } | ||
|
|
||
| case MigrationAction.Optionalize(at, _) => | ||
| modifyAtPath(value, at) { targetValue => | ||
| // Wrap the value in a Some variant | ||
| Right(DynamicValue.Variant("Some", targetValue)) | ||
| } |
There was a problem hiding this comment.
Mandate/Optionalize currently treat Option payloads as Variant("Some", <value>), but the schema representation for Some is a record wrapper (Variant("Some", Record("value" -> ...)); see Reflect.some* in schema/shared/src/main/scala/zio/blocks/schema/Reflect.scala). As-is, optionality migrations will not round-trip through Schema.fromDynamicValue for real Option fields. Update Optionalize to wrap in a Record(Chunk("value" -> targetValue)) and update Mandate to unwrap that record (and handle missing/invalid shapes as errors).
| * Create a selector for sequence elements. | ||
| */ | ||
| def elements[A, B]: Selector[Seq[A], B] = Elements() | ||
|
|
There was a problem hiding this comment.
elements is typed as Selector[Seq[A], B] / Elements[A, B], which allows nonsensical selectors like Selector.elements[Int, String] even though element selection should preserve the element type. Consider changing this to def elements[A]: Selector[Seq[A], A] (and final case class Elements[A] extends Selector[Seq[A], A]) to keep the selector API genuinely type-safe.
| def dropField[T](selector: A => T): MigrationBuilder[A, B] = | ||
| macro MigrationBuilderSyntaxImpl.dropFieldImpl[A, B, T] | ||
|
|
||
| /** |
There was a problem hiding this comment.
Scala 2 MigrationBuilderSyntax provides dropField(selector) but lacks an overload that accepts defaultForReverse (which exists in the core builder API and is provided in the Scala 3 syntax). Adding a dropField(selector, defaultForReverse: DynamicSchemaExpr) macro overload would keep Scala 2/3 feature parity and avoid forcing users to fall back to DynamicOptic-based calls.
| /** | |
| /** | |
| * Drop a field using a type-safe selector, providing a default expression | |
| * for reverse migrations. | |
| */ | |
| def dropField[T](selector: A => T, defaultForReverse: DynamicSchemaExpr): MigrationBuilder[A, B] = | |
| builder.dropField(MigrationBuilder.paths.from[A, T](selector), defaultForReverse) | |
| /** |
Adds comprehensive test coverage for the schema migration system to meet the 80% branch coverage minimum.
Changes
Replaces #984 (closed while CI was still running)