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

Fix: AasService Thumbnail Tests #298

Conversation

ShehriyarShariq-Fraunhofer
Copy link
Contributor

The PR Corresponds to the task titled: "Improve testing of Thumbnail CRUD operations in the AasService"

It contains a fix for the following issues:

The getThumbnailTest should not first set the thumbnail; the suite should be the one to provide an AAS with a thumbnail already set
Similarly with updateThumbnail and deleteThumbnail
deleteThumbnail should use fail pattern instead of a global test except.

@ShehriyarShariq-Fraunhofer
Copy link
Contributor Author

@mateusmolina-iese Please check now. Fixed the commit issue in this draft PR.

basyx.aasservice/basyx.aasservice-backend-inmemory/pom.xml Outdated Show resolved Hide resolved
basyx.aasservice/basyx.aasservice-core/pom.xml Outdated Show resolved Hide resolved
basyx.aasservice/pom.xml Outdated Show resolved Hide resolved
@Before
public void setUp() throws IOException {
AssetAdministrationShell expected = DummyAssetAdministrationShellFactory.create();
aasServiceWithThumbnail = getAasService(expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be createWithDefaultThumbnail

@ShehriyarShariq-Fraunhofer ShehriyarShariq-Fraunhofer marked this pull request as ready for review May 31, 2024 09:14
@zhangzai123 zhangzai123 self-requested a review June 3, 2024 11:11
@@ -244,4 +245,9 @@ public void getPaginatedAssetAdministrationShellIterating() {
public AasService getAasService(AssetAdministrationShell shell) {
return new AasRepositoryAasServiceWrapper(getAasRepository(), shell);
}

@Override
public FileRepository getFileRepository() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to overwrite it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this over here to avoid implementing it in to the child classes because for some, there is no FileRepository implementation. Returning null here just allows me to set a default FileRepository in case nothing is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -33,5 +33,10 @@
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.digitaltwin.basyx</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this dependency

@@ -65,14 +66,25 @@ public class TestMongoDBAasRepository extends AasRepositorySuite {

private static GridFsTemplate gridFsTemplate = configureDefaultGridFsTemplate(mongoTemplate);

private FileRepository fileRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialize fileRepository directly here, instead of using a constructor in the test class; Also mark it as static.

fileRepository = getFileRepository();

if (fileRepository == null) {
fileRepository = new InMemoryFileRepository();
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to define a default FileRepository in the abstract suite


<dependency>
<groupId>org.eclipse.digitaltwin.basyx</groupId>
<artifactId>basyx.filerepository-backend-inmemory</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this dep


aasService.getThumbnail();
try {
aasServiceWithThumbnail.getThumbnail();
Copy link
Contributor

Choose a reason for hiding this comment

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

use AssertThrows here

@@ -158,14 +183,13 @@ public void getPaginatedSubmodelReferencesPaginated() {

@Test
public void updateThumbnail() throws FileNotFoundException, IOException {
AssetAdministrationShell shell = DummyAssetAdministrationShellFactory.createWithDefaultThumbnail();
AasService aasService = getAasService(shell);
setAasThumbnail(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use this method?
Instead you should set using aasServiceWithThumbnail.setThumbnail, but with the dummyImageIS_B, since _A is already there

import org.eclipse.digitaltwin.basyx.core.filerepository.InMemoryFileRepository;

public class TestInMemoryAasService extends AasServiceSuite {


private FileRepository fileRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

Initialize fileRepo directly here; remove constructor; add static mod

@@ -48,14 +49,25 @@ public class TestInMemoryAasRepository extends AasRepositorySuite {

private AasBackendProvider backendProvider = new AasInMemoryBackendProvider();

private FileRepository fileRepository;
Copy link
Contributor

Choose a reason for hiding this comment

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

same thing


@Override
protected FileRepository getFileRepository() {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the consequences here?

@aaronzi aaronzi merged commit 312b3a2 into eclipse-basyx:main Jul 5, 2024
2 checks passed
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.

None yet

4 participants