betamaxpy / betamax

A VCR imitation designed only for python-requests.
https://betamax.readthedocs.io/en/latest/
Other
567 stars 62 forks source link

User typos can trigger baroque exceptions #183

Open demospace opened 4 years ago

demospace commented 4 years ago

https://github.com/betamaxpy/betamax/blob/77e61d785114a261570ff0cbf7f9834e8e4f96ff/src/betamax/adapter.py#L99

When I use a default cassette configuration with a modified match_on (running via pytest), I get an unhashable type at the line referenced above.

It looks to me like it's trying to update a dict from a list? Per the comment, I tried changing

config.default_cassette_options['match_requests_on'] = ['method', 'uri', 'jsonbody'], to config.default_cassette_options['match_requests_on'] = set(['method', 'uri', 'jsonbody']),

with no effect.

demospace commented 4 years ago

Something like: self.cassette.match_options = match_requests_on does work, but seems to fly in the face of the nearby code comment...

sigmavirus24 commented 4 years ago

We need more information to help debug this.

demospace commented 4 years ago

Groan, after further debugging above is the result of:

    with betamax.Betamax.configure() as config:
        config.cassette_library_dir = 'tests/cassettes/'
        config.define_cassette_placeholder('<BEARER_TOKEN>', BEARER_TOKEN)
        config.define_cassette_placeholder('<URI>', 'https://foo.com')
        config.default_cassette_options['serialize_with'] = 'prettyjson'
        config.default_cassette_options['match_requests_on'] = ['method', 'uri', 'jsonbody'],

    yield api

Notice the trailing comma on the match_requests_on value.

The comma makes the value a tuple, which then throws the TypeError as described above when attempting to update the set based on a tuple of (list, None).

Obviously my fault.

Some time might be saved for future souls if the type of match_requests_on is checked prior to update there and a warning with a hint to check for trailing comma provided?

I find it's trickier to debug pytests with VSCode because I haven't found a way to run the debugger against pytest with justMyCode=False. I tracked down above using print statements in the betamax lib 😮

sigmavirus24 commented 4 years ago

Some time might be saved for future souls if the type of match_requests_on is checked prior to update there and a warning with a hint to check for trailing comma provided?

I personally abhor the global configuration that I stole from VCR, and don't want to implement something that wraps dictionaries to do type-checking to be totally transparent. I'm worried, also, that checking the type before calling update later on will still hide the issue from users or make it harder to track down as there's far less locality there.

That said, I do believe there should be someway to protect users from typos like this. Perhaps if we introduce a validate method on config that can be run upon exiting the context manager or could be run explicitly?