Conversation
Member
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
This is the easy part. Getting the answer is the hard part.
apasel422
force-pushed
the
conv-http
branch
from
41307b2 to
c8a91ed
Compare
Collaborator
|
Discussed in the Oct 14 call - let's wait to merge until we have support for sending the report. |
Collaborator
Author
I think we need to deal with #189 first. |
apasel422
force-pushed
the
conv-http
branch
2 times, most recently
from
a2d8c3b to
2cb35d5
Compare
csharrison
approved these changes
Dec 2, 2025
apasel422
force-pushed
the
conv-http
branch
2 times, most recently
from
091765c to
578defb
Compare
Collaborator
Author
|
@martinthomson Please take another look. |
martinthomson
approved these changes
Dec 18, 2025
Member
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
Hopefully, these responses help.
I'll not be around for a few weeks. I'm happy for you to merge on the basis of the suggestions I've made; though if you want to discuss more, you might either have to be patient or open issues to track those.
Co-authored-by: Martin Thomson <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
martinthomson
added a commit
that referenced
this pull request
Jan 7, 2026
Co-authored-by: Martin Thomson <[email protected]>
apasel422
added a commit
that referenced
this pull request
Jan 15, 2026
* Add implementation-defined maximums This includes a bunch of values that I essentially made up. Rationale for that is listed in #174. We'll have to discuss what appropriate values are before we can merge this change. Closes #174. * Fix linking snafu * Change based on Michael's feedback * Look back 90 days * No need to null check Co-authored-by: Andrew Paseltiner <[email protected]> * Apply suggestion from @apasel422 Co-authored-by: Andrew Paseltiner <[email protected]> * error message correction Co-authored-by: Andrew Paseltiner <[email protected]> * Comment on small values * Use more obviously invalid sites in e2e tests Some systems may consider `a` to be a registrable domain. * Use valid domains in list-size e2e tests Otherwise, the test may inadvertently be covering the behavior of the invalid `a` domain, rather than the size check. * Remove unnecessary cast to Delegate * Simplify current-time construction in TestConfig * Bump Python to Bikeshed's minimum required version of 3.12 * Add JSON schema for e2e tests * Reorder validation vs test * Extract impression/conversion options into def * Require aggregationService * Add lint step to make check * Add coverage for errors at structured-headers level * Update http.test.ts Co-authored-by: Martin Thomson <[email protected]> * Add coverage for parsing impression/conversion sites/callers as sites * Enforce minimum values for non-user-input integers * Validate aggregation service format * Link directly to forgetVisits parameter * Add basic e2e test for clearing site state without forgetting visits * Add basic e2e test for clearing site state with forgetting visits * Add impression after clearing * Remove document, frame, and iframe from fetch destinations Per #189 (comment), who to associate the Save-Impression operation with is unclear in these cases. Support for them could be restored in the future if we hear of use cases and are able to resolve the association question. * Replace hardcoded "randomness" in e2e tests with configurable value To make those tests less dependent on fixed behavior. In the future, we might consider allowing these values to be specified in individual events at a higher level in order to avoid mandating the exact way in which randomness is used. * Add e2e coverage for lifetime/lookback clamping * Allow arrays of comments for readability * Update impl/e2e-tests/expiry-clamping.json Co-authored-by: Martin Thomson <[email protected]> * HTTP API for Measure Conversion (#280) Co-authored-by: Martin Thomson <[email protected]> * Add test coverage for ignoring unknown dictionary keys and parameters (#349) * Require e2e tests to conform to WebIDL definitions (#350) The WebIDL interfaces for AttributionImpressionOptions and AttributionConversionOptions specify multiple fields as either `unsigned long` or `long`, and therefore no e2e test should attempt to pass values outside these ranges, as the associated type conversion (e.g. from JavaScript) is assumed to occur strictly before the API (i.e. the simulator) receives the final values. Enforcing this at the JSON schema level ensures that test suites that import the e2e tests need not concern themselves with the WebIDL-level conversions. The change to the JSON schema revealed one such offending test case in save-impression-errors.json, which attempted to pass `histogramIndex: -1`. * Bump qs from 6.14.0 to 6.14.1 in /impl (#353) Bumps [qs](https://github.com/ljharb/qs) from 6.14.0 to 6.14.1. - [Changelog](https://github.com/ljharb/qs/blob/main/CHANGELOG.md) - [Commits](ljharb/qs@v6.14.0...v6.14.1) --- updated-dependencies: - dependency-name: qs dependency-version: 6.14.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * update editors (#355) * update editors add Ben Case as editor as discussed in editors sync. * Update api.bs * Add e2e test for single-epoch budgeting (#356) * Update the test schema --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Andrew Paseltiner <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Benjamin M. Case <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #31
Preview | Diff