Let approx() work on more generic sequences

[ndevenish]

My Problem

approx() was updated in 9f3122f to work better with numpy arrays, however at the same time the requirements were tightened from requiring an Iterable to requiring a Sequence - the former being
tested only on interface, while the latter requires subclassing or registration with the abc.

This broke our code which uses many custom containers (though it wasn't apparent to us that things were broken until the Scalar testing was tightened in cd2085e). For various (mainly political) reasons it's hard to change our code for pytest, and in any case I believe this benefits from a broader solution.

This Patch

Since the ApproxSequence only used __iter__ and __len__ anyway, this patch reduces the requirement to only what's used, and allows unregistered Sequence-like containers to be used. Since numpy arrays qualify for the new criteria, I've reordered the checks so that generic sequences are checked for after numpy arrays (which would also work if numpy every made the arrays officially sequences)

This Code

I know the new test doesn't look as neat as the old one, but thought it better to start out explicit rather than making e.g. a new custom Sequencelike abc which implements __subclasshook__ . Sequence can't be removed from compat.py because is also used for testing in

pytest/src/_pytest/assertion/util.py

Lines 113 to 114 in 176d274

	def issequence(x):
	return isinstance(x, Sequence) and not isinstance(x, basestring)

and I haven't investigated whether it's also appropriate to loosen that. I'm happy to do that work if people would require.

I've categorised the changelog as bugfix for now. Also, have followed instructions but unsure if this is large enough to qualify for the step:

• Add yourself to AUTHORS in alphabetical order;

[codecov]

Codecov Report

Merging #4327 into features will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##           features    #4327      +/-   ##
============================================
+ Coverage     95.88%   95.93%   +0.05%     
============================================
  Files           111      111              
  Lines         24997    25537     +540     
  Branches       2442     2537      +95     
============================================
+ Hits          23969    24500     +531     
- Misses          726      731       +5     
- Partials        302      306       +4

Flag	Coverage Δ
#docs	30.06% <85.71%> (+0.23%)	⬆️
#doctesting	30.06% <85.71%> (+0.23%)	⬆️
#linting	30.06% <85.71%> (+0.23%)	⬆️
#linux	95.77% <100%> (+0.05%)	⬆️
#nobyte	92.35% <93.33%> (ø)	⬆️
#numpy	91.88% <100%> (-1.26%)	⬇️
#pexpect	41.91% <100%> (+0.15%)	⬆️
#py27	92.76% <93.33%> (-1.27%)	⬇️
#py34	92.17% <93.33%> (+0.06%)	⬆️
#py35	92.18% <93.33%> (+0.06%)	⬆️
#py36	92.2% <93.33%> (+0.06%)	⬆️
#py37	94.18% <93.33%> (+0.09%)	⬆️
#trial	91.88% <100%> (-1.26%)	⬇️
#windows	94.1% <100%> (ø)	⬆️
#xdist	94.01% <100%> (+0.07%)	⬆️

Impacted Files	Coverage Δ
src/_pytest/compat.py	96.87% <100%> (ø)	⬆️
src/_pytest/python_api.py	97.43% <100%> (+0.01%)	⬆️
testing/python/approx.py	99.62% <100%> (+0.01%)	⬆️
testing/test_collection.py	99.8% <0%> (ø)	⬆️
src/_pytest/python.py	95.3% <0%> (+0.02%)	⬆️
src/_pytest/assertion/rewrite.py	95.93% <0%> (+0.23%)	⬆️
src/_pytest/config/__init__.py	95.44% <0%> (+0.43%)	⬆️
src/_pytest/capture.py	93.87% <0%> (+0.45%)	⬆️
src/_pytest/terminal.py	93.02% <0%> (+0.85%)	⬆️
testing/test_session.py	98.52% <0%> (+2.01%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62967b3...4eddf63. Read the comment docs.

[ndevenish]

Note: I realised I both left the AUTHORS change in, and got the order wrong, so fixed and re-pushed. Out of neatness habit I rewrote the main commit, so apologies for the extra work on the CI.

[RonnyPfannschmidt]

i believe his breaks if an approx object is reused - just unpack explicitly

[ndevenish]

Not entirely sure what you meant - unless you read the test class as an iterator rather than an iterable? In any case, I've split out the creations, was this what you meant?

https://github.com/pytest-dev/pytest/pull/4327/files#diff-a4d4d6cc8ecc32438c8a4da391852f22R508

[ndevenish]

I've just been reminded that lack of __iter__ doesn't mean that an object isn't iterable, as iter (and thus ranges, loops, comprehensions) will work just as well with __getitem__ only.

So technically the test should be for isinstance(expected, Iterable) or hasattr(expected, '__getitem__'). Or, attempting iter(expected) and catching the exception?

Depends how general this should be?

[nicoddemus]

IMHO, isinstance(expected, Iterable) is enough for most cases that we want to support. We may decide to be more general later if needed, but having to be more strict in the future is usually more problematic.

[nicoddemus]

I suggest using a My prefix (MyIterable) which is used in a few other places in our tests, and also follows the CamelCase naming convention for classes. 👍

[nicoddemus]

Please also rebase this on features. 👍

[nicoddemus]

If we have decided this, please specify, I suggest:

``approx`` now works with more generic containers, more precisely instances of ``Iterable`` instead of more restrict ``Sequence``.

[ndevenish]

I went for the slightly different

approx again works with more generic containers, more precisely instances of Iterable and Sized instead of more restrictive Sequence.

To emphasises the actual restriction and that it's restoring a previous compatibility

[ndevenish]

Thanks for the comments. I've

• Changed the declaration of the expected iterable
• Changed the news fragment to be more precise
• Changed to be on top of the feature branch (though should I change the news classification to a feature also?

I think I wasn't supposed to force-push the branch to physically move it to the features branch, but now I know about that GitHub feature..

[kalekundert]

It might be worth adding a test to make sure __repr__() works after making a comparison with a generator (i.e. because the generator would be consumed during the iteration, approx needs to make a copy in order to present a nice error message if the comparison fails). This might just work—I don't remember how the code is written—but it seems worth testing.

[nicoddemus]

Changed to be on top of the feature branch (though should I change the news classification to a feature also?

Yes please. 👍

It might be worth adding a test to make sure repr() works after making a comparison with a generator

Oh good catch! It is definitely worth having a test for this. 👍

[nicoddemus]

@RonnyPfannschmidt anything else here or can we merge this?