Skip to content

Conversation

@ricokahler
Copy link
Contributor

@ricokahler ricokahler commented Jun 4, 2025

This PR enhances the test suite by introducing a local patch application mechanism. This allows us to verify the correctness of the generated patches by applying them to the source document and comparing the result against the expected target document, all without needing to hit a live Sanity backend.

Key Changes:

  • New Test Helpers:
    • test/helpers/patchOperations.ts:
      • This new file contains a set of functions (set, unset, insert, diffMatchPatch, ifRevisionID) that simulate the application of Sanity patch operations to a JavaScript object.
      • These operations are based on logic adapted from the @sanity/sdk (specifically core/src/document/patchOperations.ts) to ensure consistency with how Sanity itself processes patches.
      • Includes jsonMatch and related path parsing utilities (also adapted from @sanity/sdk) to correctly target values within the document based on Sanity path expressions.
      • The KeyedSanityObject type and isKeyedObject utility were moved from src/diffPatch.ts to src/paths.ts to be accessible by these helpers and the main library code.
    • test/helpers/applyPatches.ts:
      • Introduces an applyPatches(source: DocumentStub, patches: SanityPatchMutation[]): DocumentStub helper function.
      • This function iterates through an array of SanityPatchMutation objects and uses the operations from patchOperations.ts to apply them sequentially to a source document.
  • Test Suite Integration:
    • All existing unit tests (*.test.ts files) have been updated to use applyPatches.
    • After generating patches with diffPatch, the tests now also call expect(applyPatches(source, patches)).toEqual(target) to ensure the patches transform the source into the target correctly.
  • Integration Test Modification:
    • The integration.test.ts file, which previously relied solely on dryRun commits to a Sanity backend, can now also leverage the local applyPatches if Sanity client configuration is missing. This makes the integration tests more robust and runnable in environments without backend access.
  • Fixture Update:
    • The test/fixtures/set-and-unset.ts fixture (b.arr) was updated to use null instead of undefined to align with the new undefined-to-null conversion behavior and ensure tests pass with the local patch application.

Rationale:

  • Improved Test Confidence: Directly verifying that the generated patches produce the expected outcome locally provides a much stronger guarantee of correctness than just snapshotting the patches themselves.
  • Faster Feedback Loop: Local patch application is significantly faster than relying on dryRun commits to a backend, speeding up the test suite.
  • Offline Testing: Enables more comprehensive testing even without a configured Sanity client, which is beneficial for CI environments and local development.
  • Foundation for Future Testing: These helpers provide a solid base for writing more complex tests involving patch application and merging.

Note: While this PR adds significant value to the test suite, the introduction of the patch application logic (adapted from the SDK) does add a considerable amount of code to the test helpers. If this is deemed too complex or adds too much maintenance overhead for a testing utility within this specific library, this PR could potentially be omitted, and we could continue to rely on integration tests with a backend for end-to-end verification. However, the benefits of local verification are substantial for development speed and confidence.

Copy link
Contributor Author

ricokahler commented Jun 4, 2025

@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch 2 times, most recently from a857791 to fc6ce17 Compare June 4, 2025 03:12
@ricokahler ricokahler force-pushed the v6_replace-diffitem-with-diffvalue branch from fe681e7 to 7c56fc3 Compare June 4, 2025 03:32
@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch from fc6ce17 to 20fd215 Compare June 4, 2025 03:32
@ricokahler ricokahler changed the base branch from v6_replace-diffitem-with-diffvalue to graphite-base/40 June 4, 2025 13:18
@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch from 20fd215 to 1654acc Compare June 4, 2025 13:36
@ricokahler ricokahler changed the base branch from graphite-base/40 to v6_sequential-patch-serialization June 4, 2025 13:36
@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch from 1654acc to 8f32025 Compare June 4, 2025 15:32
@ricokahler ricokahler marked this pull request as ready for review June 4, 2025 22:09
@ricokahler ricokahler requested a review from rexxars June 4, 2025 22:23
Copy link
Member

@rexxars rexxars left a comment

Choose a reason for hiding this comment

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

I feel like this is testing the SDK patch application more than this module 😅
Can we make the integration suite run both the SDK tests and the actual API integration tests? I feel strongly that we should be testing both

@ricokahler ricokahler force-pushed the v6_sequential-patch-serialization branch from dda33fd to d4a9699 Compare June 6, 2025 21:57
@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch from 8f32025 to c5e2f7f Compare June 6, 2025 21:57
- Move KeyedSanityObject interface to paths.ts to resolve circular imports
- Add applyPatches helper and patchOperations reference implementation from SDK
- Update all tests to verify patches can be applied correctly using local implementation
- Make integration tests work with or without Sanity client configuration
- Fix set-and-unset fixture to use null instead of undefined for array values

This enables testing patch correctness without requiring Sanity client setup and provides candidates for future SDK extraction.
@ricokahler ricokahler force-pushed the v6_local-patch-application-for-tests branch from c5e2f7f to e35e767 Compare June 6, 2025 22:00
@ricokahler
Copy link
Contributor Author

I feel like this is testing the SDK patch application more than this module 😅

you know, i caught a few bugs while i threw this in there and there was some motivation to see what our compliance is like 😅. in the future, i'd like to move this file into its own repo (maybe @sanity/patch-operations?) and then have integration tests similar to how they're set up here. for now, i think i'll drop this PR and remove the code. will remove this from the stack and restack.

@ricokahler ricokahler closed this Jun 6, 2025
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.

2 participants