Add mock impl for Adobe Granite Asset API #49
Conversation
|
thanks for the contribution! i applied some cosmetic changes - format code following our standes, add license headers, eliminate some code warnings. sonarcloud analysis fails due to lack of coverage - can you check https://github.com/wcm-io/io.wcm.testing.aem-mock/pull/49/checks?check_run_id=28941901554 and add a few more unit tests? thanks! |
|
Sure, I'll have a look. Thanks for the fast reply. |
| return (AdapterType) resource; | ||
| } | ||
| //to be able to adapt to granite asset | ||
| if (type == com.adobe.granite.asset.api.Asset.class) { |
There was a problem hiding this comment.
You can also implement this in the AdapterFactory, which might be a bit cleaner?
There was a problem hiding this comment.
makes it a lot harder to access inner resources and assets. I'd rather not.
core/src/main/java/io/wcm/testing/mock/aem/dam/MockGraniteAssetWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/wcm/testing/mock/aem/dam/MockAemDamAdapterFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/wcm/testing/mock/aem/dam/MockGraniteAssetManagerWrapper.java
Outdated
Show resolved
Hide resolved
| assertNull(resource); | ||
|
|
||
| damEvent = damEventHandler.getLastEvent(); | ||
| assertTrue(damEvent.isPresent()); |
There was a problem hiding this comment.
Simplifying DamEventHandler would result in the isPresent() and get() calls being removed, which saves some boilerplate
There was a problem hiding this comment.
existing code so out of scope for this rather small pr?
core/src/test/java/io/wcm/testing/mock/aem/dam/MockGraniteAssetWrapperTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Test | ||
| public void testStream() { | ||
| assertNotNull(rendition.getStream()); |
There was a problem hiding this comment.
I think Rendition#getStream is a @NotNull-method? It would be nice to verify the actual result value, i.e. by saying which bytes it should be, or that it should be empty (given the testSize-method below here)
|
|
||
| @Test | ||
| public void testSize() { | ||
| assertEquals(0L, rendition.getSize()); |
There was a problem hiding this comment.
Are we testing here with a 0 byte rendition?
| assertEquals("original", rendition.getName()); | ||
| assertEquals("/content/dam/sample/portraits/scott_reynolds.jpg/jcr:content/renditions/original", rendition.getPath()); | ||
| assertEquals("image/jpeg", rendition.getMimeType()); | ||
| assertNotEquals(0, rendition.hashCode()); |
There was a problem hiding this comment.
This check is not super useful, I would prefer assertEquals(383948394, rendition.hashCode()), or is the hashcode not constant? (Because that would be an issue)
There was a problem hiding this comment.
the rendition tests are copied from the existing rendition tests (same methods/logic anyway). I did not go into detail about what they actually do.
This goes for this remark and the other(s) for the MockGraniteRenditionTest class
There was a problem hiding this comment.
the original testing code which inspired this is also already very old and not the best one. so, although not ideal, it's fine for me for duplicating it here for the moment. we could improve it here and in the original position in a separate PR.
|
thanks to @henrykuijpers for the in-depth review! a few cosmetic points are already fixed by my reformatting, most are still relevant. |
|
I tried to add tests for all logic but it's missing some coverage on all the "unsupported oepration" lines ofcourse. My IDE says 78% total coverage, curious to see how much coverage on new lines now... |
it still reports 77%. we do not have to test unsupported operations, and also not error handling for exotic edge cases. but i will add a comment or two where i think a coverage is missing or i have an open question. |
core/src/main/java/io/wcm/testing/mock/aem/dam/MockGraniteAssetWrapper.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/wcm/testing/mock/aem/dam/MockGraniteAssetWrapper.java
Outdated
Show resolved
Hide resolved
…type property (which is mandatory) delegate implementation of Granite Asset.setRendition to this method
Even though it's not the recommended API for assets, the Adobe Granite Asset API has some useful methods compared to the Day CQ Asset API (like removeAsset).
We use it in some projects (especially the removeAsset functionality).
This PR adds mocks for this.
I opted to use wrappers for the existing mocks (
MockAsset&MockAssetManager) instead of re-implementing the same or similar code.MockRenditionalready had the required methods so that was solved by just adding the other interface.