Skip to content
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

Add e2e tests #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

osmanhadzic
Copy link

Add end-to-end test for:

Send property from device to server and from server to device
Send Datastream from device to server and from server to device
Send Aggregate data from device to server and from server to device

Closes: #54

tests/e2etest/GenericTest/build.gradle Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
package org.astarteplatform.devicesdk.tests.e2etest.utilities;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need this interface and GenericDeviceMockDataFactory class? I think you can use GenericDeviceSingleton directly, for e2etest, it is a simple use case for testing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My factory pattern is for call mock data and a singleton class, which I think is an easy way to read the code.

@harlem88 harlem88 requested a review from lucaato December 5, 2023 14:30
Signed-off-by: Osman Hadzic <[email protected]>
Copy link
Collaborator

@lucaato lucaato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed some files, let me know what you think about the comments.

Comment on lines +58 to +63
interfaceServerData = "org.astarte-platform.java.e2etest.ServerDatastream";
interfaceDeviceData = "org.astarte-platform.java.e2etest.DeviceDatastream";
interfaceServerAggr = "org.astarte-platform.java.e2etest.ServerAggregate";
interfaceDeviceAggr = "org.astarte-platform.java.e2etest.DeviceAggregate";
interfaceServerProp = "org.astarte-platform.java.e2etest.ServerProperty";
interfaceDeviceProp = "org.astarte-platform.java.e2etest.DeviceProperty";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these interface names be static final variables ?
I would also check other fields that could be made static and constant, it's cleaner in my opinion and this will make for a lighter constructor.

Comment on lines +30 to +35
mockDevice.getInterfaceDeviceAggr(),
mockDevice.getInterfaceDeviceData(),
mockDevice.getInterfaceDeviceProp(),
mockDevice.getInterfaceServerAggr(),
mockDevice.getInterfaceServerData(),
mockDevice.getInterfaceServerProp()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion you should rename this class, currently it's called GenericInterfaceProvider but it has hardcoded interfaces for a specific device.
The better alternative imho would be to have the interfaces list initialized in the constructor and store this generic interface provider inside GenericMockDevice, from what I've seen of the code it shouldn't be a problem and would separate concerns better.

return new JSONObject(0);
}

public void postServerInterface(String interfaces, String endpoint, Object payload)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we use different terms, what do you think about renaming the function parameters to postServerInterface(String interface, String path, Object payload) ? I would also rename the arguments of the other methods of this class by following this convention.


import org.astarteplatform.devicesdk.AstarteDevice;

public class GenericDeviceMockDataFactory implements GenericDeviceMockData {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you only have one implementation of this class you could just avoid the interface and keep this class with static methods that return the fields you need. But since the interface got used pretty heavily throughout the code you can also choose to keep it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants