Update ChargePoint tests to use new constructor with dependency injection. #789
Conversation
There was a problem hiding this comment.
Does this also include the test fixture renames (that wasn't clear from the PR description)? If so, can we please change the base to that branch so that it is easier to review this PR? With the current setup, it is hard to determine which changes are related to this refactor and which are related to the prior rename, which makes it harder to review.
| ocpp::v201::Callbacks create_callbacks_with_mocks() { | ||
| ocpp::v201::Callbacks callbacks; | ||
|
|
||
| callbacks.is_reset_allowed_callback = is_reset_allowed_callback_mock.AsStdFunction(); |
There was a problem hiding this comment.
please clarify why these were removed? I don't see where they were replaced (at least while skimming through the changes), and I don't see any explanation in the PR about why they are no longer needed.
There was a problem hiding this comment.
create_callbacks_with_mocks() created a Callbacks object, then setup the mocks on it, then returned it.
But we already have a Callbacks object in the test fixture, with a function that is used to setup the mocks on it.
These functions were almost identical, except one used the fixture's Callbacks object and one created a new one. So I eliminated the need for one of them by using the other.
|
No, this PR is not based off of the test fixture renames work. I broke up the test fixture into smaller, easier to understand fixtures, and I named those using the new naming convention. This entire PR is its own refactoring unrelated to the test rename refactoring PR. |
lib/ocpp/v201/charge_point.cpp
Outdated
| @@ -148,6 +148,9 @@ ChargePoint::ChargePoint(const std::map<int32_t, int32_t>& evse_connector_struct | |||
| v2g_certificate_expiration_check_timer([this]() { this->scheduled_check_v2g_certificate_expiration(); }), | |||
| callbacks(callbacks), | |||
| stop_auth_cache_cleanup_handler(false) { | |||
| EVLOG_info << "This ChargePoint constructor should no longer be used in favor of the one that allows for " | |||
There was a problem hiding this comment.
Since we're relying on c++17, did you consider using the attribute [[deprecated]] (see https://en.cppreference.com/w/cpp/language/attributes/deprecated). This warning you will get at compile time, instead of runtime and might be more appropriate w.r.t. API usage.
There was a problem hiding this comment.
Actually I wasn't sure if we had standardized on what to use to indicate deprecation. I have a question out in Zulip: https://lfenergy.zulipchat.com/#narrow/stream/417678-EVerest.3A-Framework-.26-Tools/topic/Deprecation.20procedure.3F
But I can make that change if that's what the project typically does. I am also not sure if the expectation is that the API/code is stable enough to require deprecation warnings or not.
There was a problem hiding this comment.
I am also not sure on what we would like to use here so would like to get this resolved before merging.
Otherwise the PR looks good.
There was a problem hiding this comment.
My preference would be to remove the old constructors entirely, but I only hesitate because I'm worried that someone is using them somewhere.
There was a problem hiding this comment.
I think that would be acceptable as long as its easy to migrate to the other constructors. There are breaking changes on a regular basis within libocpp so I think this one should be allowed as well.
@hikinggrass what do you think?
There was a problem hiding this comment.
@marcemmers @hikinggrass @a-w50 Since we don't currently have a deprecation strategy formally in place, and since this code base is not considered stable enough to worry about breaking changes, I'm inclined to remove the old constructors, which also allows for the ChargePoint class to be cleaned up a bit more. The main risk is if someone somewhere is using libocpp and the ChargePoint constructors. Other than changing documentation, I'm not sure how to communicate to anyone using the original constructors that they'll need to use the new one.
Any concerns with this approach?
There was a problem hiding this comment.
I think that's reasonable, like @marcemmers said there's still breaking changes from time to time at the moment anyways
gberardi-pillar
requested review from
Pietfried,
hikinggrass,
marcemmers and
maaikez
as code owners
gberardi-pillar
force-pushed
the
feature/k01_chargepoint_refactor
branch
3 times, most recently
from
991121c
to
33b926d
Compare
…tion. Once all old constructors no longer are used in other areas of the code, then they can be removed, ensuring the new constructor is used exclusively. Also, the tests have been split up to use multiple test fixtures that cater to the kind of functionality being tested to make it easier to read and understand for future maintenance. Signed-off-by: Gianfranco Berardi <[email protected]>
gberardi-pillar
force-pushed
the
feature/k01_chargepoint_refactor
branch
from
33b926d
to
c916a85
Compare
Signed-off-by: Gianfranco Berardi <[email protected]>
Once all old constructors no longer are used in other areas of the code, then they can be removed, ensuring the new constructor is used exclusively.
Also, the tests have been split up to use multiple test fixtures that cater to the kind of functionality being tested to make it easier to read and understand for future maintenance.
This work also had the nice side effect of eliminating some flakiness resulting from the fixture creating a TestChargePoint object and some tests creating a separate ChargePoint, as when both were doing cleanup, it sometimes resulted in segfaults related to database connections.
Checklist before requesting a review