✨ (rc): Start BLE + Start Battery Service #508
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
File comparision analysis report🔖 Info
Click to show memory sections📝 SummaryClick to show summary
🗺️ Map files diff outputClick to show diff list
|
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
from
bc8280c
to
da4d16d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #508 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 101 96 -5
Lines 1566 1475 -91
=========================================
- Hits 1566 1475 -91
Continue to review full report at Codecov.
|
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
from
da4d16d
to
d4b8347
Compare
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
3 times, most recently
from
9cfef5d
to
347b790
Compare
ladislas
approved these changes
Mar 7, 2022
| @@ -27,11 +29,12 @@ class RobotController : public interface::RobotController | |||
|
|
|||
| void raise(auto event) { state_machine.process_event(event); }; | |||
|
|
|||
| void initializeComponents() { _ble.init(); } | |||
There was a problem hiding this comment.
est-ce qu'on peut pas le faire dans le constructeur? plutôt que d'appeler une fonction qu'on peut oublier d'appeler ? ou alors le ctor appelle la fonction privée initializeComponents.
je me pose la même question pour registerEvents
There was a problem hiding this comment.
On a déjà eu dans le passé des cas où le constructeur qui devait appeler des méthodes privées ne le faisait pas à l'exécution. On peut reprendre le pari d'essayer.
There was a problem hiding this comment.
On a déjà eu dans le passé des cas où le constructeur qui devait appeler des méthodes privées ne le faisait pas à l'exécution
on se souvient desquels et de pourquoi ça marchait pas?
ça serait intéressant de comprendre pourquoi, s'il fallait modifier un truc de notre côté ou si c'est "comme ça".
si pas le choix, on vivra avec. mais si c'est possible, ça rend l'interface et l'utilisation beaucoup plus simple et propre.
There was a problem hiding this comment.
There was a problem hiding this comment.
Après, c'est aussi pour les tests: c'est plus simple de focaliser sur ce qu'on attend de chaque fonctions au lieu d'avoir tous les appels et de devoir faire un EXPECT_CALL pour les unexpected calls
On peut les garder sous forme de fonctions, mais ça peut prêter à confusion d'avoir des fonctions publique qu'on appelle pas dans l'exécution du programme et uniquement en test
Edit. En fait c'est aussi le cas pour toutes les autres méthodes de RobotController, des méthodes qui ne sont appelées que par les tests et pas dans le main.cpp. Donc ça ne gênera pas d'avoir des méthodes publiques non appelé dans main.cpp
There was a problem hiding this comment.
Aussi, il semblerait que les tests soient aussi cassés
Même si les EXPECT_CALL sont définis, ils ne sont pas bien pris en compte à l'exécution des tests et on se retrouve avec 2 types d'erreurs: d'un côté il y a des uninterresting calls et de l'autre des EXPECTED_CALL qui ne sont pas appelés.
Je vais voir s'il y a quelque chose à faire, dans ce cas je supprimerai ce commentaire
Edit. J'ai une solution, mais elle n'est pas très "propre" puisqu'elle impose 2 lignes supplémentaires dans chaque test.
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
from
3bd95f0
to
dfb719d
Compare
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
from
dfb719d
to
2a44e1a
Compare
YannLocatelli
force-pushed
the
yann/feature/robot-controller/start-ble
branch
from
2a44e1a
to
f6b5006
Compare
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.