Remove Fledge/Protected Audience tests [chromium/src : main]
Russ Hamilton (Gerrit)
Russ Hamilton added 9 comments![]( "Open in Gerrit")
I changed the approach so that instead of making tests that reference Fledge pass, I just deleted them. I also split this CL so that this CL only performs test deletions. Changing to use stubs will be in the next CL.
Russ Hamiltonnit: Shouldn't be multiple many blank lines in a row. Looks like this leaves three. Should only be one.
Done
// beacons are set up in chrome/test/data/interest_group/bidding_logic.js toRuss HamiltonThis mentions an interest group test file, but is testing beacons, and as far as I can tell, this test doesn't actually load the referenced files....And tests beacons, not IGs, or IG code to send a beacon. Am I missing something?
It was deleted because it uses NavigateFencedFrameUsingFledge, which runs an auction.
void NavigateFencedFrameUsingSharedStorage(Russ HamiltonI feel like this test rework belong in a separate CL. I don't know the shared storage stuff, and this is new code in the middle of a massive CL. It's test only, but UI tests have a tendency towards flake, and I don't know the code being tested at all.
I reworked the CL to delete affected tests instead of trying to fix them.
scoped_refptr<FencedFrameReporter> CreateFencedFrameReporter(Russ HamiltonThis does less than the other modified test I called out, but I do feel this should also be separated out.
Acknowledged
blink::features::kBrowsingTopicsDocumentAPI,
blink::features::kFencedFrames, network::features::kSharedStorageAPI,Russ HamiltonIs it ok to remove these? Admittedly, if all features being removed at once, may make sense to just remove all in one CL.
I'm trying to limit the scope of this CL, so I just want to affect Fledge here.
class FledgeFencedFrameOriginContentBrowserClientRuss HamiltonWe make enough changes to this file that I think it should also be separated out (also seems weird we're keeping a FencedFrameFoo class here. Should we be renaming it?
Acknowledged
// This test exercises restrictions on fenced frame sizes in opaque-ads mode.Russ HamiltonThis talks a lot about opaque-ads mode...but I don't see it actually setting opaque-ads mode on a frame, or running an auction. Does this actually test any PA API this CL removes, or just FencedFrames stuff specifically intended for use with PA?
It uses NavigateFencedFrameUsingFledge, which runs an auction.
TEST_F(FencedFrameReporterTest, CustomDestinationURLNoOrEmptyAllowlist) {Russ HamiltonUnclear to me if some of these tests are PA specific, or just use PA as a test case. I guess FencedFrame + FencedFrameReporters are going away, anyways?
They use `FencedFrameReporter::CreateForFledge`, so they get deleted.
Related details
- mmenke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 2 comments![]( "Open in Gerrit")
Quick response - this is an issue that affect a lot of these tests removals. I'm not well enough plugged into just what's going away to know if deleting a lot of these tests is ok.
TEST_F(FencedFrameReporterTest, CustomDestinationURLNoOrEmptyAllowlist) {Russ HamiltonUnclear to me if some of these tests are PA specific, or just use PA as a test case. I guess FencedFrame + FencedFrameReporters are going away, anyways?
They use `FencedFrameReporter::CreateForFledge`, so they get deleted.
That doesn't answer the question. Yes, they remove CreateForFledge(), but they don't exist CreateForFledge(). e.g., if I'm removing the bookmarks button from the toolbar, and all my overflow tests just happen to use the bookmarks button, those tests should not be deleted, but instead updated to use one of the other buttons instead.
I want to know if these are testing something that will still exist - that is, some aspect of reporting related to the interaction of FencedFrame and FencedFrameReporters. If those APIs will still be around, these need to be carefully reviewed to make sure we aren't losing test case coverage. If FencedFrame and/or FencedFrameReporters are going away, the issue becomes moot.
Related details
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 2 comments![]( "Open in Gerrit")
I changed the approach so that instead of making tests that reference Fledge pass, I just deleted them. I also split this CL so that this CL only performs test deletions. Changing to use stubs will be in the next CL.
Acknowledged
TEST_F(FencedFrameReporterTest, CustomDestinationURLNoOrEmptyAllowlist) {Russ HamiltonUnclear to me if some of these tests are PA specific, or just use PA as a test case. I guess FencedFrame + FencedFrameReporters are going away, anyways?
mmenkeThey use `FencedFrameReporter::CreateForFledge`, so they get deleted.
That doesn't answer the question. Yes, they remove CreateForFledge(), but they don't exist CreateForFledge(). e.g., if I'm removing the bookmarks button from the toolbar, and all my overflow tests just happen to use the bookmarks button, those tests should not be deleted, but instead updated to use one of the other buttons instead.
I want to know if these are testing something that will still exist - that is, some aspect of reporting related to the interaction of FencedFrame and FencedFrameReporters. If those APIs will still be around, these need to be carefully reviewed to make sure we aren't losing test case coverage. If FencedFrame and/or FencedFrameReporters are going away, the issue becomes moot.
I'm not sure what portions of FencedFrames will continue to exist, so I'm going to revert this file. We'll have to decide what to do with them when the time comes to remove CreateForFledge and CreateForSharedStorage.
Related details
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
TEST_F(FencedFrameReporterTest, CustomDestinationURLNoOrEmptyAllowlist) {Russ HamiltonUnclear to me if some of these tests are PA specific, or just use PA as a test case. I guess FencedFrame + FencedFrameReporters are going away, anyways?
mmenkeThey use `FencedFrameReporter::CreateForFledge`, so they get deleted.
Russ HamiltonThat doesn't answer the question. Yes, they remove CreateForFledge(), but they don't exist CreateForFledge(). e.g., if I'm removing the bookmarks button from the toolbar, and all my overflow tests just happen to use the bookmarks button, those tests should not be deleted, but instead updated to use one of the other buttons instead.
I want to know if these are testing something that will still exist - that is, some aspect of reporting related to the interaction of FencedFrame and FencedFrameReporters. If those APIs will still be around, these need to be carefully reviewed to make sure we aren't losing test case coverage. If FencedFrame and/or FencedFrameReporters are going away, the issue becomes moot.
I'm not sure what portions of FencedFrames will continue to exist, so I'm going to revert this file. We'll have to decide what to do with them when the time comes to remove CreateForFledge and CreateForSharedStorage.
Update: These are all for Fenced Frame Ads Reporting which *will* be going away. So they will be removed in a later CL.
mmenke (Gerrit)
mmenke added 13 comments![]( "Open in Gerrit")
Did not take a close look at all the primarily PA-focused files, looking more at the other tests that we may need to keep.
IN_PROC_BROWSER_TEST_F(AutomaticBeaconCredentialsBrowserTest,
3PCEnabledAndDisabled) {I'm not qualified to know if parts of this test are still relevant.
AutomaticBeaconDestinationEnrollment) {Is enrollment for these FLEDGE-only, or a general beacon thing? I see AttestedApiStatus::kSharedStorage and AttestedApiStatus::kAttributionReporting. I believe shared storage is going away, but I'm not sure about kAttributionReporting, nor if that might mean whether or not we still need to test something here.
The same is true of all these enrollment tests, until we get to the more clearly FLEDGE-specific ones (ReportWinDestinationEnrollment, ReportResultDestinationEnrollment)
Anyhow, I don't feel qualified to sign off on these three enrollment tests.
SameOrigin_Enrolled_Success) {Is aggregated reporting going away entirely? If not, and we still have some concept of enrollment for it, we ma still need this. I suspect aggregated reporting is going away, but am not sure.
// Test that automatic beacons are sent after clicking "Open Link in New Tab"
// from a contextual menu inside of a fenced frame.This sounds like a fenced frame + beacon test (that uses an auction, but may still make sense outside the context of an auction).
ReportEvent_ReportSent) {Not sure about the future of fenced frame and attribution reporting. Goes for all 3 of these tests.
TEST_F(FencedFrameReporterTest, CustomDestinationURLNoOrEmptyAllowlist) {Russ HamiltonUnclear to me if some of these tests are PA specific, or just use PA as a test case. I guess FencedFrame + FencedFrameReporters are going away, anyways?
mmenkeThey use `FencedFrameReporter::CreateForFledge`, so they get deleted.
Russ HamiltonThat doesn't answer the question. Yes, they remove CreateForFledge(), but they don't exist CreateForFledge(). e.g., if I'm removing the bookmarks button from the toolbar, and all my overflow tests just happen to use the bookmarks button, those tests should not be deleted, but instead updated to use one of the other buttons instead.
I want to know if these are testing something that will still exist - that is, some aspect of reporting related to the interaction of FencedFrame and FencedFrameReporters. If those APIs will still be around, these need to be carefully reviewed to make sure we aren't losing test case coverage. If FencedFrame and/or FencedFrameReporters are going away, the issue becomes moot.
I'm not sure what portions of FencedFrames will continue to exist, so I'm going to revert this file. We'll have to decide what to do with them when the time comes to remove CreateForFledge and CreateForSharedStorage.
Sorry, that should have been "they don't exist 'to test' CreateForFledge()". I'm going to go through and look specifically for anything that I'm not confident is a test we'll no longer care about once FLEDGE is removed.
return attachFencedFrameContext({
attributes: [["allow", "shared-storage"]]Is allow a concept outside of shared-storage (which I know is also going away)?
});I'm going to skip the rest of this directory. I don't feel qualified to know if the combination of these features (the \*ad\* files I'm happy to sign off on, the rest just leave me scratching my head)
//
// This test whether the two credentialless iframe can communicate and bypass the
// fencedframe boundary. This shouldn't happen.Also not sure about anonymous iframes - I know nothing about them, so again, skipping this directory.
const config = await generateURNFromFledge(I suspect we should keep this test, but switch how it navigates the fenced frame.
Concerned these may still be relevant in the non-FLEDGE case
<title>Test Private Aggregation doesn't work in cross-origin frames.</title>If Private Aggregation is not being deprecated, concerned about the first two of these files...I think it's going away, though?
Related details
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
- Andrew Verge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Verge (Gerrit)
Andrew Verge voted and added 4 comments![]( "Open in Gerrit")
Votes added by Andrew Verge
| Code-Review | +1 |
4 comments
LGTM for fenced frames.
RE @mme...@chromium.org's comments RE fenced frames tests: After PA and Shared Storage are removed there will be no valid way to populate a fenced frame. So it should be safe to clean up these tests.
IN_PROC_BROWSER_TEST_F(AutomaticBeaconCredentialsBrowserTest,
3PCEnabledAndDisabled) {I'm not qualified to know if parts of this test are still relevant.
From a fenced frames perspective, automatic beacon tests should be safe to clean up. Fine with this being removed.
return attachFencedFrameContext({
attributes: [["allow", "shared-storage"]]Is allow a concept outside of shared-storage (which I know is also going away)?
The allow attribute for frames is used to specify their Permissions Policy: https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/allow
I'm going to skip the rest of this directory. I don't feel qualified to know if the combination of these features (the \*ad\* files I'm happy to sign off on, the rest just leave me scratching my head)
I'll handle the fenced-frame/ review, resolving this to unblock.
Related details
- Russ Hamilton
- mmenke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 9 comments![]( "Open in Gerrit")
Adding cshar...@chromium.org for attributions_browsertest and ads_page_load_metrics_observer
Adding k...@google.com for chrome/browser/privacy_sandbox/*
AutomaticBeaconDestinationEnrollment) {Is enrollment for these FLEDGE-only, or a general beacon thing? I see AttestedApiStatus::kSharedStorage and AttestedApiStatus::kAttributionReporting. I believe shared storage is going away, but I'm not sure about kAttributionReporting, nor if that might mean whether or not we still need to test something here.
The same is true of all these enrollment tests, until we get to the more clearly FLEDGE-specific ones (ReportWinDestinationEnrollment, ReportResultDestinationEnrollment)
Anyhow, I don't feel qualified to sign off on these three enrollment tests.
These tests are about enrollment which is also going away.
SameOrigin_Enrolled_Success) {Is aggregated reporting going away entirely? If not, and we still have some concept of enrollment for it, we ma still need this. I suspect aggregated reporting is going away, but am not sure.
Yes, aggregated reporting is going away.
// Test that automatic beacons are sent after clicking "Open Link in New Tab"
// from a contextual menu inside of a fenced frame.This sounds like a fenced frame + beacon test (that uses an auction, but may still make sense outside the context of an auction).
This form of fenced frames are going away, so it should be fine to remove this test.
ReportEvent_ReportSent) {Not sure about the future of fenced frame and attribution reporting. Goes for all 3 of these tests.
This file will be removed entirely in crrev.com/c/7958319, so deleting them in this CL is likely okay.
//
// This test whether the two credentialless iframe can communicate and bypass the
// fencedframe boundary. This shouldn't happen.Also not sure about anonymous iframes - I know nothing about them, so again, skipping this directory.
Per ave...@chromium.org these can be removed.
const config = await generateURNFromFledge(I suspect we should keep this test, but switch how it navigates the fenced frame.
Per ave...@chromium.org these can be removed.
Concerned these may still be relevant in the non-FLEDGE case
Per ave...@chromium.org these can be removed.
<title>Test Private Aggregation doesn't work in cross-origin frames.</title>If Private Aggregation is not being deprecated, concerned about the first two of these files...I think it's going away, though?
- Charles Harrison
- Kevin Graney
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charles Harrison (Gerrit)
Charles Harrison voted Code-Review+1![]( "Open in Gerrit")
- Kevin Graney
- Russ Hamilton
- mmenke
mmenke (Gerrit)
mmenke voted and added 1 comment![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
1 comment
LGTM, thanks for doing this!
Related details
- Kevin Graney
- Russ Hamilton
Kevin Graney (Gerrit)
Kevin Graney voted Code-Review+1![]( "Open in Gerrit")
- Russ Hamilton
Russ Hamilton (Gerrit)
Russ Hamilton added 1 comment![]( "Open in Gerrit")
Adding p...@chromium.org for VirtualTestSuites and content_shell.filter
Adding a...@chromium.org for browsing_data_model_browsertest.cc, declarative_net_request_browsertest.cc, render_view_context_menu_interactive_uitest.cc, and run_py_tests.py
Related details
- Avi Drissman
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Graney (Gerrit)
Message from Kevin Graney![]( "Open in Gerrit")
(Note: This is an automated comment left by Gemini)
There are a few oversights and dangling references in this CL where tests were deleted but their associated configuration lines were left behind:
1. Dangling `TestExpectations` Entries The CL deletes several Fledge-related fenced-frame tests, but fails to remove their expectation overrides in `third_party/blink/web_tests/TestExpectations`. Leaving these in place will likely cause the test expectation linter or bot to complain. The following deleted tests still have entries:
- `virtual/fenced-frame-mparch/external/wpt/fenced-frame/fledge-container-size.https.html`
- `virtual/fenced-frame-mparch/external/wpt/fenced-frame/automatic-beacon-click-handler.https.html`
- `virtual/fenced-frame-mparch/external/wpt/fenced-frame/set-automatic-beacon.https.html`
2. Dangling `TestLists` Entry for `external/wpt/fledge`
The CL deletes the entire `external/wpt/fledge/` directory, but leaves a reference to it in `third_party/blink/web_tests/TestLists/rel-ready.blink_wpt_tests.filter`.
3. Missed Cleanup for Retired Parakeet Tests (Nit / Likely Oversight)
The commit message mentions deleting `virtual/parakeet/` (the virtual test suite for the retired Parakeet API). However, the base test files remain untouched in `third_party/blink/web_tests/external/wpt/parakeet/` (along with their `*-expected.txt` baseline failure files). Since Parakeet is retired and the virtual suite testing it was deleted, the base WPTs should probably be deleted as well to complete the cleanup.
Related details
- Avi Drissman
- Philip Rogers
- Russ Hamilton
Kevin Graney (Gerrit)
Kevin Graney added 1 comment![]( "Open in Gerrit")
Message
Marking the thread as unresolved.
1 comment
Parent: 06eef046 ([Vertical Tabs] Add context menu to tab group header)Marking the previous feedback as unresolved. Please address the oversights mentioned in the previous comment.
Related details
- Avi Drissman
- Philip Rogers
- Russ Hamilton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement