[CQT-466] Increase test coverage - #709
Conversation
elenbaasc
changed the title
| def test_eq_only_one_has_rotations(self, semantic_with_rotations: CanonicalGateSemantic) -> None: | ||
| assert semantic_with_rotations != CanonicalGateSemantic((0.25, 0.25, 0.25)) | ||
| assert CanonicalGateSemantic((0.25, 0.25, 0.25)) != semantic_with_rotations |
There was a problem hiding this comment.
I don't understand the point of these asserts?
There was a problem hiding this comment.
They are meant to check that __eq__ is symmetric.
The guard in canonical_gate.py is self.rotations is None or other.rotations is None. I thought that whichever object sits on the left of the != is the one that becomes self, so swapping the order would mean that each assert covers a different half of that or.
Maybe I should clearly rename it to test_eq_is_symmetric_when_only_one_has_rotations? Or do you think the test is just redundant and I should get rid of it?
There was a problem hiding this comment.
Indeed the l.h.s. is self and the r.h.s. is the other. Nevertheless, I would say that this test is redundant, because there is no asymmetry in the current implementation and it would be rather strange to introduce one later on.
| def test_repr(self) -> None: | ||
| assert repr(IR()) == "IR: []" |
There was a problem hiding this comment.
Would be more interesting (better test), to add some statements. This will force one to make sure to define a __repr__ method for each statement.
There was a problem hiding this comment.
Thank you for the suggestion Chris! Indeed, this does make the test a lot more meaningful. I have applied your suggestion, for Barrier, Init and Measure. While I was at it, I noticed that SingleQubitGate and AsmDeclaration don't implement __repr__, and neither do any of their bases (i.e. Unitary, Gate, Statement etc.), so they fall back to object.__repr__.
Should I go ahead and add the __repr__ method to e.g. SingleQubitGate and AsmDeclaration, and then add the relevant statements to test_repr?
There was a problem hiding this comment.
Yes, it would be good to add __repr__s to the ones that don't have one. Fine to do it in this PR. The AsmDeclatation, might be and interesting one, because I think we should not include the content in the representation. But have a look if you think it should.
2 participants
No description provided.