Closed apahim closed 5 years ago
Merging #146 into master will increase coverage by
<.01%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #146 +/- ##
==========================================
+ Coverage 88.01% 88.02% +<.01%
==========================================
Files 25 25
Lines 2561 2563 +2
==========================================
+ Hits 2254 2256 +2
Misses 307 307
Impacted Files | Coverage Δ | |
---|---|---|
msrest/serialization.py | 91.05% <100%> (+0.01%) |
:arrow_up: |
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 adb4596...748148d. Read the comment docs.
@apahim can you explain your scenario? I understand concept of length for list, dict, and so on, but for a random object? @johanste any thoughts?
Hi @lmazuel, I was checking the azure-mgmt-compute
code (azure-sdk-for-python) and noticed that there is no assertion in the test method test_mgmt_compute.MgmtComputeTest.test_vm_extension_images
. A minimal test there would be to assert the length of the object returned by compute_client.virtual_machine_extension_images.get()
. That object is an instance of VirtualMachineExtensionImage
, which inherits from Resource
, which inherits from this object. Without this patch, we cannot assert the len()
of VirtualMachineExtensionImage
(unless we add it in one of the more specialised children classes). But I believe it makes sense to include this code here, considering it's an object that serves as data structure and it already uses self.__dict__
for __eq__
, __ne__
and __str__
. It's safe to assume that the length of this object is the length of its __dict__
. After/if this patch gets in, the next step would be to send the diff below to azure-sdk-for-python:
commit 6219fa270facfd97b07751e6f1e6d26d8a73b65c (HEAD -> assert_vm_extension_images, origin/assert_vm_extension_images)
Author: Amador Pahim <amador@pahim.org>
Date: Tue Feb 19 00:55:40 2019 +0100
Adding assertion for the test_vm_extension_images
`VirtualMachineExtensionImage` now supports len(), so let's use that to
assert that we have results for our request.
Signed-off-by: Amador Pahim <amador@pahim.org>
diff --git a/azure-mgmt-compute/tests/test_mgmt_compute.py b/azure-mgmt-compute/tests/test_mgmt_compute.py
index 793eb07c8..a4fdcd346 100644
--- a/azure-mgmt-compute/tests/test_mgmt_compute.py
+++ b/azure-mgmt-compute/tests/test_mgmt_compute.py
@@ -402,6 +402,7 @@ class MgmtComputeTest(AzureMgmtTestCase):
type_name,
version,
)
+ self.assertGreater(len(result_get), 0)
return
def test_vm_images(self):
Conceptually, I'm not a huge fan of stretching __len__
for that usage. If we follow the Python doc itself:
len(o)
Return the length (the number of items) of an object. The argument may be a sequence (such as a string, bytes, tuple, list, or range) or a collection (such as a dictionary, set, or frozen set).
I don't feel object here follows the description of sequence/collection.
Because it's doable (let's be honest, nothing's impossible in Python :)) doesn't man it's a good idea :)
Also, your len test is not correct anyway, since at worst the attributes will exists with None value, so your test will be always True
Agreed with @lmazuel. The proposed change and your description on how you intend to use it are inconsistent. And, as @lmazuel pointed out, it does not follow the expected semantics of len.
It sounds like what you are trying to do is to access the raw json response.
Hi, thank you for the review. Yes, I was trying to keep the test simple, but later I noticed that this object will have at least one attribute (self.additional_properties = {}
) on __init__
(if not a xml_model) and saying that the length of its __dict__
is the length of the object is not really accurate. And I agree that this object does not look like something that should behave like a sequence and, as such, I have no intention of proposing the sequence protocol implementation for it (__len__
+ __getitem__
). I will instead make a proper testing on the sdk side. Cheers!
to check the Model length we just have to check the its
__dict__
length.