Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Upgrade testing infrastructure #513

Open
wants to merge 91 commits into
base: main
Choose a base branch
from
Open

chore: Upgrade testing infrastructure #513

wants to merge 91 commits into from

Conversation

ok-nick
Copy link
Contributor

@ok-nick ok-nick commented Jul 17, 2024

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

Checklist

  • 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 ok-nick added enhancement New feature or request testing Issues or pull requests that modify test functionality labels Jul 17, 2024
sdk/Cargo.toml Outdated Show resolved Hide resolved
@gpeacock
Copy link
Collaborator

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.

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

⚠️ 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 ⚠️

❗ 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ok-nick ok-nick marked this pull request as ready for review August 6, 2024 13:34
@ok-nick
Copy link
Contributor Author

ok-nick commented Aug 6, 2024

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 ok-nick requested a review from mauricefisher64 August 9, 2024 14:24
sdk/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@gpeacock gpeacock left a comment

Choose a reason for hiding this comment

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

It looks like it all works - I can make a small change and see it is caught. But I think we need more instruction about how to use and maintain it. My concern is that when the spec changes the format of a UUID, these will break, and we will need to fix the tests. So how do we deal with expected changes going forward?

@ok-nick
Copy link
Contributor Author

ok-nick commented Aug 15, 2024

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
Copy link
Contributor Author

ok-nick commented Aug 29, 2024

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.

@scouten-adobe scouten-adobe changed the title Upgrade testing infrastructure chore: Upgrade testing infrastructure Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request testing Issues or pull requests that modify test functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants