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

refactor: test suite #94

Merged
merged 1 commit into from
Feb 7, 2021
Merged

refactor: test suite #94

merged 1 commit into from
Feb 7, 2021

Conversation

tplooker
Copy link
Contributor

This PR starts the refactor of the current plugfest test suite over to a new package that features a more generalized testing infrastructure that is runnable on an HTTP server to allow for on demand testing of register vendor APIs. Still very much a WIP and is based on the hard work of @OR13 on the did-core-test-suite.

Changes of note

  • No net change in core test cases, the tests that were being performed before are still the same
  • Generalized fixtures, with the plugfest test suite, the fixtures were vendor specific when they should really be shared to ensure there aren't divergent implementations
  • New interop test cases added, for instance with the VC verify endpoint a set of tests will run against a filtered list of the fixtures based on things like the did methods the vendor expresses support for in their config.

Whilst converting over to this format I was only able to get results from Digital Bazaars and Transmutes APIs at present, the rest appeared to be offline?

I still have quite a bit to do on this PR, but looking for early feedback primarily from @OR13 due to his familiarity with it.

peacekeeper pushed a commit to peacekeeper/vc-api that referenced this pull request Jan 31, 2021
Co-authored-by: Scott Malley <srmalley-sphereon@github.com>
Copy link
Member

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

This looks good to me, although @OR13 is of course better qualified to comment on the depths of the JavaScript code and overall structure.

@peacekeeper
Copy link
Member

I was only able to get results from Digital Bazaars and Transmutes APIs

Danube Tech's endpoints are up, but the Issuer DIDs have changed. I can update that in a separate PR after this is merged.

module.exports = [
{
name: "USCIS Permanent Residency Card",
data: require('./case-1.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider recursive load from filesystem, so that new stuff does not need to be manually registered.

name: "Permanent Residency Card",
issuerDidMethod: "did:key:",
linkedDataProofSuite: "Ed25519Signature2018",
data: require('./case-1.json')
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider recursive load from fs instead off manual registration.

/**
* Configuration for running the test suite
*/
module.exports = [
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want to wire these to CI, so that each vendor is shown as passing / failing on each PR, please create an issue to track this requested feature

@@ -0,0 +1,32 @@
module.exports = extractTestSummary = (results) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this structure looks valuable to did core, please open an issue on that suite to track alignment with changes here.

});

return summary.sort((a, b) =>
JSON.stringify(a).localeCompare(JSON.stringify(b))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider canonicalize instead of stringify.

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

This PR is large, please open issues to track a few comments, then I suggest we merge. great work @tplooker

@mprorock
Copy link
Contributor

mprorock commented Feb 5, 2021

question on merge timeframe - I would like to start writing docs around the tests so that we can get them in prior to impending code freeze - how long are we waiting to get this merged?

@tplooker
Copy link
Contributor Author

tplooker commented Feb 5, 2021

Apologies on this, really busy week, I am working on some updates but don't expect to have them complete for a few days, perhaps we should merge and I will file a separate PR to address the outstanding issues.

@OR13
Copy link
Contributor

OR13 commented Feb 7, 2021

multiple reviews, open for 7 days... merging.

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.

5 participants