tests: use markers to configure disable_subp_usage#473
Conversation
|
The use of markers here isn't strictly required, but some pending test work I have will use them in a way that will require their use; I tested the practicality of this approach by refactoring this code, and it results in cleaner test code so I proposed it independent of that pending work. |
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
This is a vast improvement and much cleaner IMO.minor nit and +1.
| def side_effect(args, *other_args, **kwargs): | ||
| raise AssertionError("Unexpectedly used subp.subp") | ||
|
|
||
| elif allow_subp_for is not None and allow_all_subp is not None: |
There was a problem hiding this comment.
Can we keep the ordering of variable checks consistent as it makes me think harder when reviewing this
| elif allow_subp_for is not None and allow_all_subp is not None: | |
| elif allow_all_subp is not None and allow_subp_for is not None: |
There was a problem hiding this comment.
Oh yeah, my bad! Fix pushed.
This is an improvement over indirect parameterisation for a few reasons: * The test code is much easier to read, the mark names are much more intuitive than the indirect parameterisation invocation, and there's less boilerplate to boot * The fixture no longer has to overload the single parameter that fixtures can take with multiple meanings
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
Sorry, late to the party! But I agree, this is a nice change and the code look a lot nice.
| from cloudinit import subp | ||
|
|
||
|
|
||
| def _closest_marker_args_or(request, marker_name: str, default): |
There was a problem hiding this comment.
is this supposed to be _closest_marker_args_or_default ?
There was a problem hiding this comment.
I'd be happy to land such a change if you want, but (unsurprisingly, as I wrote it 😁) I think its intent is clear (and means that callers can generally call it on a single LOC; though that isn't much of an advantage if this spelling isn't clear!).
If we do want to change it, _get_closest_marker_args might be better, following the underlying API (and dict.get which takes a default)?
There was a problem hiding this comment.
No need to change it. I think it's fine. I just wanted to make sure it was what you intended and not an accidental misnaming.
There was a problem hiding this comment.
Also, I would like to point out the irony of me pointing out a possible typo, while at the same time writing the code look a lot nice. 😄
3 participants
This is an improvement over indirect parameterisation for a few reasons:
intuitive than the indirect parameterisation invocation, and there's
less boilerplate to boot
fixtures can take with multiple meanings