Skip to content

Conversation

@xuganyu96
Copy link
Contributor

This PR closes #2279

ACVP test vectors will now be fetched from https://raw.githubusercontent.com/usnistgov/ACVP-Server/refs/tags/v1.1.0.40/gen-val/json-files/ at test time using Python's requests library. Previously stored JSON files and fetch_values.sh scripts were removed. I also ran a Python code formatter.

Some considerations for reviewers:

  • Speaking from principles, external resources (i.e. the JSON file) should be set up as pytest.fixture so that a single instance of a resource is not fetched repeatedly across tests. However, from past experience I found fixtures to be a pain to debug, and all but one JSON file is used exactly once, so I chose to not use fixtures
  • Currently the JSON files are fetched from the website instead of GitHub API. Should this be a concern?
  • [NO] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [NO] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged. Also, make sure to update the list of algorithms in the continuous benchmarking files: .github/workflows/kem-bench.yml and sig-bench.yml)

@xuganyu96 xuganyu96 linked an issue Oct 22, 2025 that may be closed by this pull request
@xuganyu96 xuganyu96 added this to the 0.16.0 milestone Oct 22, 2025
@coveralls
Copy link

coveralls commented Oct 22, 2025

Coverage Status

coverage: 83.601% (+0.01%) from 83.589%
when pulling 9de3977 on gyx-2279-remove-git-tracked-acvp-test-vec
into ed5c2cc on main.

baentsch
baentsch previously approved these changes Oct 23, 2025
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

From just looking at the code, this seems OK to me. Thanks @xuganyu96 !

I chose to not use fixtures

Sounds sensible to me.

Currently the JSON files are fetched from the website instead of GitHub API. Should this be a concern?

My only concern would have been that that website may be unreachable when CI runs -- but given this is also on GH and not some US gov server (that may be subject to shutdown (in various meanings of the word :), I'd say it's highly unlikely for this to be a problem. My take for now: Let's see how this performs and whether we need to improve later (eg, using GH caching).

@xuganyu96 xuganyu96 force-pushed the gyx-2279-remove-git-tracked-acvp-test-vec branch from 0795a83 to 9de3977 Compare October 24, 2025 19:42
@ashman-p
Copy link
Contributor

ashman-p commented Nov 6, 2025

I also share @baentsch concerns about NIST server accessibility. It adds another variable to trouble shoot in case of a test failure.

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.

Fetch ACVP test vectors from source instead of storing them

4 participants