[1.27.0] Tests for custom data types

[Karrenbelt]

Proposed changes

Tests that showcase issues with custom data types that implement their own __eq__ and __lt__ methods.

In furtherance of deterministic config serialization - #359

There are several issues with the implementation of __dunder__ methods in our code base. Here we showcase those with respect to comparison operators: __eq__, __ne__, __lt__, __le__, __gt__, __ge__.

open-aea/aea/configurations/data_types.py

Lines 119 to 128 in 5aa7427

	def __lt__(self, other: Any) -> bool:
	"""Compare with another object."""
	enforce(
	isinstance(other, PackageVersion),
	f"Cannot compare {type(self)} with type {type(other)}.",
	)
	other = cast(PackageVersion, other)
	if self.is_latest or other.is_latest:
	return self.is_latest < other.is_latest
	return str(self) < str(other)

open-aea/aea/configurations/data_types.py

Lines 462 to 493 in 5aa7427

	def __lt__(self, other: Any) -> bool:
	"""
	Compare two public ids.
	>>> public_id_1 = PublicId("author_1", "name_1", "0.1.0")
	>>> public_id_2 = PublicId("author_1", "name_1", "0.1.1")
	>>> public_id_3 = PublicId("author_1", "name_2", "0.1.0")
	>>> public_id_1 > public_id_2
	False
	>>> public_id_1 < public_id_2
	True
	>>> public_id_1 < public_id_3
	Traceback (most recent call last):
	...
	ValueError: The public IDs author_1/name_1:0.1.0 and author_1/name_2:0.1.0 cannot be compared. Their author or name attributes are different.
	:param other: the object to compate to
	:raises ValueError: if the public ids cannot be confirmed
	:return: whether or not the inequality is satisfied
	"""
	if (
	isinstance(other, PublicId)
	and self.author == other.author
	and self.name == other.name
	):
	return self.package_version < other.package_version
	raise ValueError(
	"The public IDs {} and {} cannot be compared. Their author or name attributes are different.".format(
	self, other
	)
	)

open-aea/aea/configurations/data_types.py

Lines 657 to 659 in 5aa7427

	def __lt__(self, other: Any) -> bool:
	"""Compare two public ids."""
	return str(self) < str(other)

• Expected failures:

  -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
  ============================================================================================= short test summary info =============================================================================================
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[list0-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_self_type_comparison[PackageId] - TypeError: '<=' not supported between instances of 'PackageId' and 'PackageId'
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[tuple-PackageId] - assert False
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[PackageId-PublicId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[str-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_public_id_comparison - TypeError: '<=' not supported between instances of 'PublicId' and 'PublicId'
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[float-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[complex-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[int-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_package_version_comparison - assert False
  FAILED tests/test_configurations/test_data_types.py::test_self_type_comparison[Dependency] - TypeError: '<' not supported between instances of 'Dependency' and 'Dependency'
  FAILED tests/test_configurations/test_data_types.py::test_self_type_comparison[PublicId] - ValueError: The public IDs C/A:0.0.0:Qm00000000000000000000000000000000000000000000 and J/A:0.0.0:Qm00000000000000000...
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[list1-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[PackageId-Dependency] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[zip-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[Dependency-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[PublicId-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  FAILED tests/test_configurations/test_data_types.py::test_package_id_comparison - TypeError: '<=' not supported between instances of 'PackageId' and 'PackageId'
  FAILED tests/test_configurations/test_data_types.py::test_type_comparison[map-PackageId] - Failed: DID NOT RAISE (<class 'TypeError'>, <class 'ValueError'>)
  

Issues

1. Stricter equality comparison is needed: abolish the use of string casting for comparsion in __lt__.
   Use VersionInfo for everything relating to comparisons in PackageVersion, PublicId and PackageId. It has many useful features implemented for us.
2. functools.total_ordering missing on PublicId and PackageId.
3. We should not return False if other is not an instance of type(self) in __eq__ and __lt__ operators. We should opt to raise when comparing apples to oranges.
   • We do this for PackageVersion.__lt__ and PublicId.__lt__, but not in PackageId.__lt__.
   • We don't deflect in __eq__ methods.
4. PackageId.__lt__ is most problematic. Not only does it compare the string representation of PublicId, containing a string representation of the version, but it also compares against the string representation of any arbitrary object. It should raise an error. We should deal with sorting necessary for serialization in the serialization process.

Suggested changes:

• Delegate all comparison to semver.VersionInfo. __eq__ and __lt__ must first check for "any" and "latest" in __lt__: if self.version_like == other.version_like: return False else self.version_like < other.version_like
• PublicId.version should be a property returning an instance of semver.VersionInfo
• __eq__ should deflect when not isinstance(other, type(self)) by returning NotImplemented (see #383).
• PackageVersion.__lt__ should be / rely on PackageVersion._version (which should be an instance of semver.VersionInfo)
• PublicId.__lt__ should raise on incoherent type, author or name, otherwise delegate to semver.VersionInfo
• PackageId.__lt__ should raise on incoherent type or package_type, otherwise delegate to PublicId
• add the functools.total_ordering decorator to PublicId and PackageId.
• Sorting for deterministic serialization can be achieved via json.
  • PackageVersion.json: return self.version_like.to_tuple() # or a nasty "any" / "latest" string
  • PublicId.json: return dict(author=..., name=..., version=self.package_version.json, package_hash=...)
  • PackageId.json: return dict(package_type=..., **self.public_id.json)

This would constitute a breaking change. The usage of "any" and "latest" complicates matters unnecessarily, we may consider changing this prior to next major release.

Types of changes

What types of changes does your code introduce to agents-aea?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

• I have read the CONTRIBUTING doc
• I am making a pull request against the develop branch (left side). Also you should start your branch off our develop.
• Lint and unit tests pass locally with my changes and CI passes too
• I have added tests that prove my fix is effective or that my feature works
• I have checked that code coverage does not decrease.
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

[Karrenbelt]

equal to setup in open-autonomy setup.cfg

[codecov-commenter]

Codecov Report

Base: 90.29% // Head: 90.29% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (8ed2410) compared to base (8c684c4).
Patch coverage: 94.23% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #428   +/-   ##
=======================================
  Coverage   90.29%   90.29%           
=======================================
  Files         352      352           
  Lines       28700    28721   +21     
=======================================
+ Hits        25916    25935   +19     
- Misses       2784     2786    +2     

Flag	Coverage Δ
unittests	90.29% <94.23%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aea/configurations/data_types.py	99.18% <93.02%> (-0.82%)	⬇️
aea/cli/check_packages.py	96.81% <100.00%> (ø)
aea/helpers/base.py	100.00% <100.00%> (ø)
aea/helpers/dependency_tree.py	100.00% <100.00%> (ø)
plugins/aea-cli-ipfs/aea_cli_ipfs/ipfs_utils.py	100.00% <0.00%> (+0.58%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Karrenbelt]

not all mypy types are covered in the strategies listed here. E.g. in case of SimpleIdOrStr: Union[SimpleId, str], a string should adhere to the pattern of SimpleId or will otherwise be invalid.

[Karrenbelt]

some raise custom ValueError, I suggest raise TypeError on all instead.

[Karrenbelt]

this seems to me the desired type of behaviour.

[Adamantios]

🚀

[Adamantios]

LGTM, let's stack a fix PR.

[Karrenbelt]

TODO: introduce Comparable type annotation, see: python/typing#59

[solarw]

lgtm
coool!

[solarw]

check for instance is done cause will fail on Version() == str() ?

[solarw]

lgtm