-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored GPIO to be testable and gradle upgrade #116
base: master
Are you sure you want to change the base?
Conversation
gradle deprecated features must be updated
1)set gpio to low 2)set gpio to high
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were the version upgrades of gradle, junit, mockito necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this version upgrade necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we commit code (dependency imports and source code) in comments?
If I may interrupt, I think version upgrades are not that big a deal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the individual comments and recommendations.
@@ -57,13 +55,17 @@ | |||
|
|||
private TimerTask _inputListenerTask; | |||
|
|||
private static CommandWriter WRITER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this field static instead of instance level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the field will be instance-level, then please follow the lowercase naming convention also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the GIPO Manager is a Singleton class and I want to set this writer from outside of the manager. So anyway it will have 1 instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But here the Gpio is neither a singleton nor a static-only class. So its methods and fields should be instance level.
That's another question, why is GpioManager
a Singleton class with static-only methods? It should be either singleton or static-only. Mixing the two does not make too much sense.
.../components/gpiomanager/src/main/java/hu/bme/mit/inf/modes3/components/gpiomanager/Gpio.java
Outdated
Show resolved
Hide resolved
w.flush(); | ||
} | ||
Logger.info(TAG, "Succeeded!"); | ||
public static void setWriter(CommandWriter writer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this setter instance-level, instead of class-level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes instance level setter any sense with Singleton GPIO manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because Gpio itself is neither Singleton, nor static-only. Here we are talking about Gpio.java and not GpioManager.java.
.../components/gpiomanager/src/main/java/hu/bme/mit/inf/modes3/components/gpiomanager/Gpio.java
Outdated
Show resolved
Hide resolved
private static CommandWriter WRITER; | ||
|
||
private static CommandReader READER; | ||
|
||
public Gpio(int pin, Direction direction) throws IOException, GpioNotConfiguratedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initialization of READER and WRITER are missing here. You may end up having a NullPointerException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I got in this case (with static variables) a nullpointer, that is definitely a misconfiguration (you must set those before usage), so the exception must occur. Should I rather instantiate the variables with GPIOWriter/Reader when the GPIO manager is instantiated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should always make sure, that whenever the constructor is invoked, all the fields and variables that this constructor uses (invokes) must have been instantiated.
Having a separate setter of a field and making sure that that setter must have been invoked before anyone creates an instance of Gpio
is very dangerous. (You rely on someone else to invoke those setters before any instance of this class is created.)
infos.put(targetFile.substring(targetFile.lastIndexOf("/") + 1), value); | ||
} catch (Exception e) { | ||
// TODO: handle exception | ||
throw e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do proper error handling.
@@ -41,6 +42,8 @@ class TrackElementController implements ITrackElementController, PhysicalTurnout | |||
config = Configuration::loadPinoutConfig(turnoutID, factory) | |||
pinout = ExpanderConfigInterpreter.loadPinoutConfig(factory) | |||
GpioManager::loadGpioMappingFromFile(GPIO_JSON) | |||
// TODO Check validity | |||
GpioManager::setGpioWriter(new GpioWriter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is only the GpioWriter instantiated here? What about GpioReader?
|
||
import hu.bme.mit.inf.modes3.components.gpiomanager.Gpio.Level; | ||
|
||
public class TestGpioReader implements CommandReader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a separate class created for test, instead of using Mockito and creating a Mock in the test case itself?
If you just return the same static value every time, then a Mock might be more suitable instead of a generally named TestGpioReader
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it can be refactored.
import java.util.HashMap; | ||
import java.util.Map; | ||
|
||
public class TestGpioWriter implements CommandWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for having a 'dedicated' TestGpioWriter
class instead of just mocking the corresponding field by mockito in the unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it can be refactored.
} | ||
|
||
@Override | ||
public void executeCommand(String value, String targetFile) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method invoked by different value
and targetFile
parameters in the unit tests? If so, then does it that unit test test this class itself or it is just a side-effect that this method will also be invoked and executed when you test another method (of another class) which invokes this class?
So, is there a reason why this 'test' class was implemented instead of just mocking it in the class/instance where it is invoked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments and recommendations.
// assert | ||
assertAll("properties", () -> assertEquals("in", tgw.getInfos().get("direction")), | ||
() -> assertEquals(datas[0].port, Integer.parseInt(tgw.getInfos().get("export"))) | ||
// ,() -> assertEquals("both", tgw.getInfos().get("edge")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we commit code in comments?
() -> assertEquals("0", tgw.getInfos().get("value"))); | ||
} | ||
|
||
// @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we commit code in comments?
A general question in the end: did you test the changes together with the modules (components) which use the |
General answer: First of all I did not have yet opportunity and the knowledge to test it, but its true, its necessary before the merge. |
@zsoltmazlo There are two issues which might arise:
|
} | ||
|
||
public static void setGpioWriter(CommandWriter writer) { | ||
Gpio.setWriter(writer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, why do you have to invoke Gpio.setWriter here? It has nothing to do with the GpioManager class. Gpio.setWriter coud be invoked anywhere else in the code.
(The same applies for the setGpioReader function also.)
I wanted to use JUnit 5 and for that I had to upgrade the versions. |
@ecsedigergo Argument is accepted. If it does not cause any compilation (/ runtime) problems, then the version upgrade is good to go. |
No description provided.