Convert to betterproto
#37
Conversation
github-actions
bot
added
part:tests
Yes, also fixing the missing |
llucax
force-pushed
the
betterproto
branch
2 times, most recently
from
36bf487
to
c3ec230
Compare
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Some notable changes: * The imports can be greatly simplified as now we can import whole modules and avoid clashes without having to create one alias per symbol. * We don't need to cast awaitable functions anymore as betterproto has the correct type hints * Because of the above, a bug was uncovered and fixed, where an `await` was missing for the API call to `AddInclusionBounds`. I'm not sure if this call was not working at all or just errors wouldn't be detected. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Since betterproto use type hints, it expects the `ConnectionFilter` `starts` and `end` to be `list`s. We might also just take a `list` in the `connections()` methods instead to avoid the copy, maybe ensuring that the filter elements are unique (which was probably the reason to make them a `set`) is not that important. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
These tests need just minor changes to the imports. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This only adds unnecessary extra indentation. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
In future commits we'll remove the testing grpc server in favour of just mocking the stub, so we don't need to really listen to a port and include in the unit tests all the gRPC machinery, when we only want to test the client does what it is supposed to do when getting some data from the stub. This also removes another level of extra indentation. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Fixing this tests is quite complicated because we also stop using a real gRPC server (`mock_api`) for the tests, to avoid introducing too much noise and flakiness to *unit tests* (see frequenz-floss/frequenz-sdk-python#662 for example). For now we just tried to replace `mock_api` with `unittest.mock` mocks, doing the minimal work to get the tests working and move forward with the migration to betterproto, but in the future we'll provide a higher level API client mock (see frequenz-floss#39). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The timeouts are now tested as part of gRPC failure tests in the client tests, since timeouts are handled by `grpclib` there isn't much we can specifically test about timeouts that is not just testing `grpclib` itself. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We don't use this module anymore and we don't plan to use it for now as it is very flaky. In the future we might use something similar to do integration tests. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
|
This will not pass the tests because it is still waiting for a release of microgrid-betterproto, but it should be ready for the final review, as no code changes should be needed. |
|
Now everything is green 💚 |
|
Enabling auto-merge. |
| requires = [ | ||
| "setuptools == 68.1.0", | ||
| "setuptools_scm[toml] == 7.1.0", | ||
| "frequenz-repo-config[lib] == 0.9.1", |
There was a problem hiding this comment.
Including repo-config as a build dependency in every project was actually a mistake, it was only needed for API projects and now that this is taken care by setuptools-betterproto (if we do the switch), it will not be used by any project. My idea is to split repo-config even further in the future, it was an experiment and now that we have a much clear picture about what we need from it, it is a good time to split it into smaller projects and use those smaller projects only where necessary.
There was a problem hiding this comment.
It is unrelated to this PR, though, I just did it now because I realized it was a mistake to include repo-config in the build dependencies right after creating setuptools-betterproto.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
| @@ -390,7 +382,7 @@ def from_proto(cls, raw: PbComponentData) -> Self: | |||
| raw_power = raw.inverter.data.ac.power_active | |||
| inverter_data = cls( | |||
| component_id=raw.id, | |||
| timestamp=raw.ts.ToDatetime(tzinfo=timezone.utc), | |||
There was a problem hiding this comment.
Hmm we have functions for the conversion in the client library
There was a problem hiding this comment.
This comes from the SDK, predates all the recent work we've been doing with APIs.
This PR migrated the Microgrid API to use
betterprotoinstead ofgrpc.A couple of bugs were found on the way and fixed, one of them directly related to the lack of proper type hints in
grpc.The changes are fairly small.
Note
This PR is just a draft, tests need to be adapted still and I don't plan to merge this to
v0.x.xdirectly, as it would be an unnecessary breaking change for the branch used by the SDK. This is mainly to show what changes are needed to migrate a project tobetterproto.This PR is using an unreleased repository that hosts the files generated by betterproto from the protobuf files. It will be published soon ™️