c2pa-rs icon indicating copy to clipboard operation
c2pa-rs copied to clipboard

chore: Upgrade testing infrastructure

Open ok-nick opened this issue 1 year ago • 5 comments

Changes in this pull request

Upgrade testing infrastructure.

  • Use insta-rs for snapshot testing
  • Test reading old c2pa-rs version manifests are compatible with the current version
    • CI runs each publish and calls c2pa-compat which generates embedded/remote/json manifests for one of every type of asset handler (e.g. jpeg, bmff, png, gif, etc.).
      • Assets are binary diffed against original assets and compressed (only storing modified manifest blocks).
      • The json manifest used for signing should cover all possible features that c2pa-rs supports in the current version.
    • CI sends PR with new compat snapshots for the current version.
    • Integration test runs against all compat snapshot versions and verifies reading all the old c2pa-rs version manifests are exactly the same as what's stored.
    • Ensures compatibility and tests against regressions.
  • CI changes
    • Add back code coverage.
    • Insta snapshot test reporting.
    • Deny cargo doc warnings.

Related Issues

  • #451
  • #396
  • #412

Checklist

  • [x] This PR represents a single feature, fix, or change.
  • [ ] All applicable changes have been documented.
  • [ ] Any TO DO items (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.

ok-nick avatar Jul 17 '24 19:07 ok-nick

it feels redundant having tests/fixtures/assets/. We should ether put the assets folder in the tests folder or put the contents of assets into fixtures. It looks like it is missing the fixtures/certs folder, so I can't run tests.

gpeacock avatar Jul 18 '24 21:07 gpeacock

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 146 lines in your changes missing coverage. Please review.

Project coverage is 79.96%. Comparing base (1a2711a) to head (16b9bc2). Report is 66 commits behind head on main.

Files Patch % Lines
c2pa-compat/src/main.rs 0.00% 146 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   79.62%   79.96%   +0.34%     
==========================================
  Files          87       90       +3     
  Lines       27276    30048    +2772     
==========================================
+ Hits        21719    24029    +2310     
- Misses       5557     6019     +462     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jul 22 '24 15:07 codecov-commenter

~~Blocked by issues with SVG and RIFF, but otherwise ready.~~ ~~Blocked by #546~~ ~~Blocked by #551~~

Disclaimer: Since SVG is a text format, when the CI downloads the repo, git will convert the line endings to CRLF (due to core.autocrlf). There are some weird results with the SVG/XML parser when mixing line endings that cause the test to fail. However, this is fixed by setting core.autocrlf=false in CI.

ok-nick avatar Aug 06 '24 13:08 ok-nick

As far as stabilizing values like UUIDs go, it should be a simple change to the Stabilizer. However, if the format of the JSON were to change, there can be some issues.

In the case where new fields are added to the JSON, the test currently ignores them by default. In the case where the location of the field changes or is removed, there’s currently no way to specify that. Ideally, we’d have a system built on top that tells us the known changes/additions/removals from the prior version.

That sounds simple enough, but there’s cases where a field may change that’s inside of an array of maps, we’d need a way to specify that as well. Insta-rs has a cool way of doing this with their redaction selectors, but that code isn’t decoupled. Unfortunately, we can’t leverage insta-rs here because redaction selectors must be hardcoded (and due to other limitations). If we want this type of functionality, we’d need to implement our own selector syntax, although I’m unsure of the difficulty and if we can leverage any existing crates.

For the time being and for the simplest functionality, we can ignore any unknown fields that don’t exist in both JSONs and verify that existing fields have the same exact values. However, if we can guarantee that fields will never be removed/moved, then we can definitely create a simple impl for known changes.

TLDR; I'll clarify maintenance in the docs and there is a lot of potential for making this suite stricter

ok-nick avatar Aug 15 '24 13:08 ok-nick

FYI, CI is failing due to a weird issue with snapshot filtering, where it isn't filtering the current version used in the PR here. It doesn't happen locally, my thought is that it's using the latest c2pa-rs version only in CI somehow? I believe it can be temporarily fixed by merging main into this branch, but that shouldn't be necessary.

ok-nick avatar Aug 29 '24 17:08 ok-nick