fix(test): call configure() on script addons in taddons.context.configure - #8369
Open
citizen204 wants to merge 1 commit into
Open
fix(test): call configure() on script addons in taddons.context.configure#8369citizen204 wants to merge 1 commit into
citizen204 wants to merge 1 commit into
Conversation
…gure context.script() returns an addon that is registered but not part of the addon chain, so the options.changed broadcast that normally triggers configure() for chained addons never reaches it. context.configure() now invokes configure() on it directly after updating options. Fixes mitmproxy#3402
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
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.
Summary
context.script()returns an addon that isregister()-ed but never added toAddonManager.chain. Theoptions.changed->_configure_all->trigger()broadcast that normally callsconfigure()after an option change only walksself.chain, so a script addon'sconfigure()is silently never called again after the initial load — even thoughcontext.configure(addon, some_option=...)updates the option value itself just fine. This makes it impossible to write a test that exercises a script addon'sconfigure()reaction to an option change.Reproduced on current
mainwith a minimal script addon that records itsconfigure()calls: aftertctx.configure(addon, foo='bar'), the option updates butconfigure()is never invoked a second time.Changes
mitmproxy/test/taddons.py:context.configure()now explicitly invokesConfigureHookon the target addon after updating options, but only when that addon isn't already part ofself.master.addons.chain— chained addons (the common case, e.g.context(some_addon)) already receive the hook via the existingoptions.changedbroadcast, so this avoids callingconfigure()twice for them.test/mitmproxy/data/addonscripts/configure_recorder.py: new fixture script addon that records everyupdatedset passed to itsconfigure().test/mitmproxy/test_taddons.py: regression test loading the fixture viacontext.script()and assertingconfigure()is called with the changed option key aftercontext.configure(). Verified the test fails on unpatchedtaddons.pyand passes with the fix.Fixes #3402