fix(core): _register_handler reenabling domains without params#253
fix(core): _register_handler reenabling domains without params#253S-Tarr wants to merge 10 commits intocdpdriver:mainfrom
Conversation
|
Hey @S-Tarr, tests are failing (I relaunched again and still failing) |
|
@nathanfallet Thanks for letting me know! I've been trying to look into the cause. I ran the tests workflow with main in my forked repo and it's also failing right now. Seems to be a different test failing every time. My best guess is that the ubuntu image used in gh actions is running chromium 146 now and the last passing workflow I've found used 145. I can't reproduce the test failures locally so it's been a bit troublesome to debug. I don't want to make anyone else have to debug my PR but do you have any idea what could cause these failures? I don't think my additions should have any effect on these tests failing. Here's a link to my forked repo's actions if you want to check. Main passed 1 in 10 attempts. My branch also passed 1 in 8 attempts. It seems like just the ubuntu tests are failing. |
|
@S-Tarr Checking because I also made another PR to update CDP protocol (wasn't updated for a while) and it fails too so might be caused by the image or something but not related to the changes (because mine passed locally) And also have the issue on the other kdriver repo while it also passed locally... |
Description
I know in @nathanfallet's description of the issue, he said the problem was that we shouldn't auto send domain enables for specific types of domains. After looking into it though, I thought it would best to have
_register_handlersrespect all manually enabled/disabled domains instead.One funny example of weird behavior I found was if I add a handler for the network domain, then manually send
network.disable, the current implementation will first enable the network domain over the web socket in_register_handlers, then send the disable transaction right after. To the user it achieves the correct result but it's a bit strange to me. A second call tosendof any kind will seenetworkinenabled_domainsalready and not try to enable it. It's not clear to me whether it should reenable it or not but it probably shouldn't be marked as enabled when disabled. If you instead first senddisablebefore adding the handler, the domain will be enabled on your next call tosend. It seems like_register_handlersneeds awareness of all enables/disables sent outside itself.My proposed additions add a new list
manually_enabled_domainsfor all calls tosendwithdomain.enable/disablenot marked with_is_update. This allows_register_handlersto hold off on accidentally overwriting any specific params like in the original issue, but also reduces unnecessary reenabling of domains.Please let me know if you have any suggestions or spot any issues. I left in a few of my tests but also let me know if they're out of place.
Hopefully this helps toward the v1.0.0 goal!
Fixes #158
Pre-merge Checklist
./scripts/format.shand./scripts/lint.shscripts. My code is properly formatted and has no linting errors.uv run pytestand ensured all tests pass.[Unreleased]section.