Commercetools/support additional types for contentful #27
Conversation
There was a problem hiding this comment.
Many improvements, I like it. Looking forward your refactoring.
I'm concerned about the IT data though. I've checked the project that we use and it's not the default data as I thought. We need to either use the default data from Contentful or explain detailed how to set up a project to run the IT or use the Contentful SDK to create the required content types and entries at the beginning of the test (if not already there). This is an open-source project and nobody would be able to collaborate if the tests cannot be run on their machine.
WDYT is the best solution?
| @Before | ||
| public void setUp() throws Exception { | ||
| contentfulCmsService = ContentfulCmsService.of(spaceId(), token(), PAGE_TYPE_NAME, PAGE_TYPE_ID_FIELD_NAME); | ||
| } |
There was a problem hiding this comment.
I like the idea of getting rid of the setUp method, so that every test can decide how to initialize the CmsService, but I would encapsulate the initialization with one or several methods.
That way, if we would change the constructor, we would only need to modify one line of code and not as many as tests we have.
There was a problem hiding this comment.
By the way, the ContentfulCmsService contains an HttpClient behind, have you checked that you are not opening a new HttpClient for every test? That would be very harmful for the test and the application.
There was a problem hiding this comment.
Can I do that in the refactoring task? I would not like to do all in this one as this was just about adding support new types... I like the idea of creating entire space content like it is done e.g. when writing tests with in-memory database with re-creating the content every time.
So TODO would be:
- extract initialization methods in it test.
- check and eventually fix HttpClient initilization so that it's reused.
- add automatic creating IT tests content every time when test is executed.
| @Test | ||
| public void whenAskForContentInArray_thenGetElement() throws Exception { | ||
| ContentfulCmsService contentfulCmsService = ContentfulCmsService.of(spaceId(), token(), "finn", PAGE_TYPE_ID_FIELD_NAME); | ||
| Optional<CmsPage> content = contentfulCmsService.page("finn", emptyList()).toCompletableFuture().get(5, TimeUnit.SECONDS); |
There was a problem hiding this comment.
I would consider not repeating yourself with the get(5, TimeUnit.SECONDS). It makes the test less readable. In the end, in these tests you always create the CmsService and request a page, right? Then my solution would be to create a function that accepts a Consumer which is the test itself. In other words:
test("finn", "finn", cmsPage -> {
assertThat(page)...
// write tests
});
private void test(final String pageType, final String pageName, final Consumer<CmsPage> test) {
// all the initialization of the CmsService and fetching of the CmsPage, plus asserting it exists
}There was a problem hiding this comment.
maybe another TODO for refactoring task?
4) refactor it tests methods
| assertThat(content).isPresent(); | ||
| Optional<String> field = content.get().field("array[0].name"); | ||
| assertThat(field).isPresent(); | ||
| assertThat(field.get()).isEqualTo("author1"); |
There was a problem hiding this comment.
With the AssertJ library we use, you can simplify it to: assertThat(field).contains("author1");
There was a problem hiding this comment.
With that, you can remove the assertThat(field).isPresent(); as it's redundant.
There was a problem hiding this comment.
you're right. I changed it because I was misled by the name of the method .contains which in String class has different semantics.
So I will change it to AssertJ's .hasValue() which is an alias for .contains() and imo is much more adequate to its behavior.
| Optional<CmsPage> content = contentfulCmsService.page("finn", emptyList()).toCompletableFuture().get(5, TimeUnit.SECONDS); | ||
|
|
||
| assertThat(content).isPresent(); | ||
| Optional<String> field = content.get().field("array[4].name"); |
There was a problem hiding this comment.
All the logic that involves the CmsPage, which has no interaction with external systems, should be done as a Unit Test to save time and resources, then you can test it more thoroughly too: cases of negative numbers, out of bounds, zero number, any valid number and other symbols too. Specially since you are working with regular expressions, you should be very carefully testing it.
| assertThat(content).isPresent(); | ||
| Optional<String> field = content.get().field("array[3].textArrayField"); | ||
| assertThat(field).isNotPresent(); | ||
| } |
There was a problem hiding this comment.
This test is very similar to whenAskForContentInArrayOutsideTheScope_thenReturnEmpty, what's the different thing you are testing here?
| public Optional<String> field(final String fieldName) { | ||
| if (StringUtils.isEmpty(fieldName)) { | ||
| public Optional<String> field(final String path) { | ||
| if (StringUtils.isEmpty(path)) { |
| * @param pathSegments array of all path segments that lead to CDAEntry of interest | ||
| * @return CDAEntry that matches path segments | ||
| */ | ||
| private Optional<CDAEntry> findEntry(final String[] pathSegments) { |
There was a problem hiding this comment.
I like it much more as Optional<CDAEntry> findEntry 👍
| * @param arrayMatcher contains key and index groups after pattern has been matched | ||
| * @return matched entry or null | ||
| */ | ||
| private Object getEntryFromArray(final CDAEntry parentEntry, final Matcher arrayMatcher) { |
There was a problem hiding this comment.
Could you annotate the method with @Nullable? To minimize the possibilities of NPE.
| int index = Integer.parseInt(arrayMatcher.group(2)); | ||
| Object field = entry.getField(arrayFieldKey); | ||
| Object item = null; | ||
| if (field != null && field instanceof ArrayList) { |
There was a problem hiding this comment.
I would rather rely on List or something less concrete.
| return type; | ||
| } | ||
|
|
||
| public static Function<Object, String> getToStringStrategy(final CDAField contentType) { |
There was a problem hiding this comment.
That get makes it a bit harder to read IMO, just toStringStrategy sounds good enough.
lauraluiz
deleted the
commercetools/support-additional-types-for-contentful
branch
#26
@lauraluiz please, review