RCA for YT adblock rule breakage (Feb 15 2024)
Closed this issue · 8 comments
We proactively added anti-YT adblock rules in #1521 but had to remove them in https://github.com/brave/adblock-lists/pull/1522/files because of reported breakage:
- https://twitter.com/epaktweets/status/1758316732626641176?s=46&t=c88ZoLNNPThnGggE75IoLQ
- https://community.brave.com/t/youtube-will-not-work-at-all-with-brave-shield-on/531816/4
We QA'ed the adblock rule prior to releasing it, so it was surprising to see widespread breakage. Some reports suggested that it only happened with Premium but that's not confirmed. Using this issue to track RCA for why the breakage happened.
@Yuki2718 did you have more luck figuring out why the breakage happened?
It was my fault, I wrongly concluded ||googlevideo.com/videoplayback*%2Cctier%2C$xhr,3p,domain=www.youtube.com blocks the same requests as the rule in easylist/easylist#17413 but this is not true. For example, Shorts have ctier=SH and %2Cctier%2C. I also subscribed Premium for this and confirmed Premium user get ctier=A and %2Cctier%2C I should have been more cautious and make the rule ||googlevideo.com/videoplayback*&ctier=L&*%2Cctier%2C$xhr,3p,domain=m.youtube.com|music.youtube.com|www.youtube.com as ctier=L is time-tested. Already done for AG https://github.com/AdguardTeam/AdguardFilters/pull/172976/files and you may want to reenable the safer rule.
Gotcha, so the problem was specifically with Premium and Shorts? Just making sure we know what to test better next time. EDIT: I can repro that adding that YT filter breaks Shorts on youtube.com.
Yes, and generally as many features and functions under various conditions should be tested.
Closing this out, we'll want to use something like brave/brave-browser#36244 in the future before rolling a filter like this out (on a risky site).
@kjozwiak do we have a runbook for YouTube testing, perhaps specifically for adblock filters? Would be worth adding Premium and Shorts to that. I can start a wiki, but just checking if @brave/qa-team has an established process for these kinda things before doing that.
Closing this out, we'll want to use something like brave/brave-browser#36244 in the future before rolling a filter like this out (on a risky site).
@kjozwiak do we have a runbook for YouTube testing, perhaps specifically for adblock filters? Would be worth adding Premium and Shorts to that. I can start a wiki, but just checking if @brave/qa-team has an established process for these kinda things before doing that.
@ShivanKaul unfortunately there's no process re: running through YT or other websites when there's an adblock/filter update. Historically QA only ever gets involved when there's an issue and we're trying to either reproduce/confirm that it's fixed. There's been some rare occasions/cases when we pushed an update and QA quickly checked to make sure everything was still working.
But in general, we never really run through any checks before any new rules are added unless it's a client change and the issue is in a milestone and labelled as QA/Yes. We would need to come up with a process about letting QA know that there's pending work that needs to be checked before pushing a new rules into production.
Are we specifically talking about YT? Or doing general regression checks? How often will these request occur? Will QA need to be involved every single time there's a new rule/update? Or just for major changes? Keep in mind that QA is pretty swamped most of the time so adding more to the workload will mean checks like these might need to get prioritized and put on hold till other work is completed (depends on where we are in the release cycle and other variables).
Regarding the rule book, we don't really have anything written down but assuming the above is related to the request made via https://bravesoftware.slack.com/archives/C3GNZANPR/p1707994772233219? I think we quickly just checked YT in that case but not 100% sure if Shorts & Premium was checked. But again, due to the workload, we basically just a few quick checks. In the future, we can definitely spend more time on this but would need a bigger heads up rather than a few hours.
That all makes sense @kjozwiak. Definitely not proposing getting QA involved every time @ryanbr pushes a new filter, but it would be very helpful for particularly risky list updates. I don't anticipate these being a common thing. Just that if/when we do, we should have a list of things we check. So for YouTube specifically, which is an important site to not break, it would be good to have a list of test cases.
That all makes sense @kjozwiak. Definitely not proposing getting QA involved every time @ryanbr pushes a new filter, but it would be very helpful for particularly risky list updates. I don't anticipate these being a common thing. Just that if/when we do, we should have a list of things we check. So for YouTube specifically, which is an important site to not break, it would be good to have a list of test cases.
Sure 👍 We could get a folder created via https://github.com/brave/qa-resources/tree/master/WikiTemplate that would include test cases/runs that we can run through when certain rules are updated/need checking. For starters, we can create the YT check and list out what should be checked/verified. We can add more runs for other changes like checking xyz (websites). CCing @brave/qa-team as a heads up/some context if they see new runs being created.