-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Digital Credentials: test create and get #46642
Conversation
There are no reviewers for this pull request besides its author. Please reach out on the chat room to get help with this. Thank you! |
let request = buildValidNavigatorIdentityRequest(); | ||
let credential = await requestIdentityWithActivation(test_driver, request); | ||
assert_equals("urn:openid.net:oid4vp", credential.protocol); | ||
assert_equals("fake_test_token", credential.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a little unfortunate that we are removing this test, since it is what's helping us avoid regressions. Why does this need to be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't adhere to the spec. For example, "urn:openid.net:oid4vp" is not a thing that we have settled on and, per spec, it's impossible to get a credential without the user interacting with the credential selection sheet (hence the promise would never resolve with a credential).
We need a separate WebDriver API that can vend credentials if we want something like this.
Obviously, the test would be fine an internal Chrome unit test, but it's not something that helps us here with interop.
Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, "urn:openid.net:oid4vp" is not a thing that we have settled on
The protocol
is a DOMString
rather than an enum
, so I wasn't thinking that we would necessarily need to settle on its value.
It is also what's being used by the OpenID4VP pull request, so a reasonable approximation of what I think we (?) would want to cover:
We need a separate WebDriver API that can vend credentials if we want something like this.
I generally agree that we need a WebDriver API, but that is fairly involved. Any chance there is a way that we can keep this test without going all the way to a WebDriver API?
Obviously, the test would be fine an internal Chrome unit test, but it's not something that helps us here with interop.
That's a reasonable compromise to me too: moving this specific test to a chrome-specific unit test as we improve the web platform tests infrastructure for the digital credential API.
This CL renames the digital-credential WPT files. The intention of this CL is to make #46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e
This CL renames the digital-credential WPT files. The intention of this CL is to make #46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <[email protected]> Reviewed-by: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1315269}
This CL renames the digital-credential WPT files. The intention of this CL is to make #46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <[email protected]> Reviewed-by: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1315269}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some review comments. Please rebase this CL with tip of tree.
…estonly Automatic update from web-platform-tests Rename digital-credential WPT files This CL renames the digital-credential WPT files. The intention of this CL is to make web-platform-tests/wpt#46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <[email protected]> Reviewed-by: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1315269} -- wpt-commits: b98b066d2f5ac7966fb5e2fffedc5f56d8e590dd wpt-pr: 46758
…estonly Automatic update from web-platform-tests Rename digital-credential WPT files This CL renames the digital-credential WPT files. The intention of this CL is to make web-platform-tests/wpt#46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <pkotwiczchromium.org> Reviewed-by: Nicolás Peña <npmchromium.org> Cr-Commit-Position: refs/heads/main{#1315269} -- wpt-commits: b98b066d2f5ac7966fb5e2fffedc5f56d8e590dd wpt-pr: 46758 UltraBlame original commit: b2d5a8c4a9170a29373da4bf0ee937741373550f
…estonly Automatic update from web-platform-tests Rename digital-credential WPT files This CL renames the digital-credential WPT files. The intention of this CL is to make web-platform-tests/wpt#46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <pkotwiczchromium.org> Reviewed-by: Nicolás Peña <npmchromium.org> Cr-Commit-Position: refs/heads/main{#1315269} -- wpt-commits: b98b066d2f5ac7966fb5e2fffedc5f56d8e590dd wpt-pr: 46758 UltraBlame original commit: b2d5a8c4a9170a29373da4bf0ee937741373550f
…estonly Automatic update from web-platform-tests Rename digital-credential WPT files This CL renames the digital-credential WPT files. The intention of this CL is to make web-platform-tests/wpt#46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <[email protected]> Reviewed-by: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1315269} -- wpt-commits: b98b066d2f5ac7966fb5e2fffedc5f56d8e590dd wpt-pr: 46758
…estonly Automatic update from web-platform-tests Rename digital-credential WPT files This CL renames the digital-credential WPT files. The intention of this CL is to make web-platform-tests/wpt#46642 easier to review as the renamed files are showing up as added+deleted in Github. BUG=5622820 Change-Id: Ia623539e5bc373e523d9ba27c930b4dab076be8e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5631578 Commit-Queue: Peter Kotwicz <[email protected]> Reviewed-by: Nicolás Peña <[email protected]> Cr-Commit-Position: refs/heads/main@{#1315269} -- wpt-commits: b98b066d2f5ac7966fb5e2fffedc5f56d8e590dd wpt-pr: 46758
f8b415a
to
4efb1cb
Compare
Let me know when you are ready for another round of code review :) |
@pkotwicz, I think these should be good to go now. Appreciate another review. |
Wanted to apologize for the long delay in the code review. My github e-mail settings were screwed up |
Let me know when you want another review on this CL |
d03f54d
to
256ff1b
Compare
904a83b
to
a5ae64b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval with nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM++
Closes WICG/digital-credentials#118
Creates a spec compliant set of test case and adds a more robust/flexible cross-origin testing framework:
Removes/refactors the old "digital-credentials/digital-credentials.tentative.https.html", as these were not spec compliant.
Adds TypeScript types, for better workflow in VS Code, etc.