Add output handler to configuration #958

FlorianKaemmerer · 2020-08-13T12:11:54Z

This is just a rough idea how outputhandlers could be added.

added FilesystemOutputHandler made OutputHandlers configurable adapted AbstractPage to use all configured OutputHandlers

removed obsolete Test

codecov-commenter · 2020-08-14T07:22:17Z

Codecov Report

Merging #958 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #958      +/-   ##
============================================
+ Coverage     97.91%   98.02%   +0.11%     
- Complexity      555      563       +8     
============================================
  Files            54       55       +1     
  Lines          1153     1167      +14     
  Branches         99      100       +1     
============================================
+ Hits           1129     1144      +15     
+ Misses            9        7       -2     
- Partials         15       16       +1

Impacted Files	Coverage Δ	Complexity Δ
...java/net/masterthought/cucumber/Configuration.java	`100.00% <100.00%> (ø)`	`44.00 <11.00> (+4.00)`
...java/net/masterthought/cucumber/ReportBuilder.java	`93.97% <100.00%> (-0.15%)`	`21.00 <1.00> (ø)`
...asterthought/cucumber/generators/AbstractPage.java	`96.22% <100.00%> (-1.78%)`	`11.00 <1.00> (+1.00)`	⬇️
...mber/json/deserializers/EmbeddingDeserializer.java	`100.00% <100.00%> (+7.14%)`	`8.00 <2.00> (+1.00)`
...cumber/outputhandlers/FilesystemOutputHandler.java	`100.00% <100.00%> (ø)`	`2.00 <2.00> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be9384f...cf2ab79. Read the comment docs.

damianszczepanik · 2020-09-04T19:53:16Z

src/main/java/net/masterthought/cucumber/Configuration.java

 import net.masterthought.cucumber.presentation.PresentationMode;
 import net.masterthought.cucumber.reducers.ReducingMethod;
 import net.masterthought.cucumber.sorting.SortingMethod;
 import org.apache.commons.lang.StringUtils;
 import org.checkerframework.checker.nullness.qual.NonNull;

+import java.io.File;
+import java.util.*;


can you revert this change?
prefer to have full name class to import

damianszczepanik · 2020-09-04T19:54:48Z

src/main/java/net/masterthought/cucumber/Configuration.java

+     *
+     * @param outputHandlerToRemove OutputHandler to be removed
+     */
+    public void removeOutputHandler(OutputHandler outputHandlerToRemove) {


not sure if we need it as long as this is used only by test

The Idea is that it can be used as a library, not every Method a library offers is used by itself.

This class is not even designed that way so probably not used that way. Please apply philosophy YAGNI

damianszczepanik · 2020-09-04T19:55:47Z

src/main/java/net/masterthought/cucumber/Configuration.java

+    /**
+     * Clears all OutputHandlers from the List of OutputHandlers
+     */
+    public void clearOutputHandlers() {


another method used only by tests

The Idea is that it can be used as a library, not every Method a library offers is used by itself.

damianszczepanik · 2020-09-04T19:55:58Z

src/main/java/net/masterthought/cucumber/ReportBuilder.java

-import net.masterthought.cucumber.generators.TagReportPage;
-import net.masterthought.cucumber.generators.TagsOverviewPage;
-import net.masterthought.cucumber.generators.TrendsOverviewPage;
+import net.masterthought.cucumber.generators.*;


damianszczepanik · 2020-09-04T19:56:13Z

src/main/java/net/masterthought/cucumber/ReportBuilder.java

-import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.IOUtils;
+
+import java.io.*;


damianszczepanik · 2020-09-04T20:04:26Z

src/test/java/net/masterthought/cucumber/ConfigurationTest.java

+    }
+
+    @Test
+    public void clearOutputHandlers_shouldRemoveAllOutputHandlersFromOutputHandlersList() {


same about convention

damianszczepanik · 2020-09-04T20:06:15Z

src/test/java/net/masterthought/cucumber/ConfigurationTest.java

+    public void clearOutputHandlers_shouldRemoveAllOutputHandlersFromOutputHandlersList() {
+        //given
+        Configuration configuration = new Configuration(outputDirectory, projectName);
+        assertThat(configuration.getOutputHandlers()).hasSize(1);


assertion should not go here
instead you can simple new handler to make sure list is not empty

removed assertion

damianszczepanik · 2020-09-04T20:07:03Z

src/test/java/net/masterthought/cucumber/ConfigurationTest.java

+    }
+
+    @Test
+    public void addOutputHandler_shouldAddGivenHandlerToListOfOutputHandlers() {


this and other method names

damianszczepanik · 2020-09-04T20:08:15Z

src/test/java/net/masterthought/cucumber/ConfigurationTest.java

+
+        //then
+        assertThat(configuration.getOutputHandlers()).hasSize(1);
+        assertThat(configuration.getOutputHandlers().get(0)).isInstanceOf(FilesystemOutputHandler.class);


you don't need to validate only class, you can validate exact reference which you have added before

updated assert

damianszczepanik · 2020-09-04T20:10:36Z

src/test/java/net/masterthought/cucumber/outputhandlers/FilesystemOutputHandlerTest.java

+        String writtenOutput = new String(Files.readAllBytes(filesystemOutputHandlerTestOutputFile.toPath()), UTF_8);
+        assertThat(writtenOutput).isEqualTo(expectedOutput);
+    }
+}


check if already added comment is not valid for other

can you also update one integration test or even add new one with memory handlers to check whether it works or not?

can I test this PR without second or both should be tested together?

Added Integration Test

You can Test it without the second PR, they are independend

updated Tests

damianszczepanik · 2020-10-14T19:24:58Z

src/main/java/net/masterthought/cucumber/Configuration.java

     */
    public void setTrends(File trendsFile, int limit) {
        this.trendsFile = trendsFile;
-        this.trendsLimit = limit;
+        trendsLimit = limit;


the formatter works strange :)
this line is updated but above doesn't

damianszczepanik · 2020-10-14T19:30:43Z

src/main/java/net/masterthought/cucumber/generators/AbstractPage.java

@@ -27,6 +17,12 @@
 import org.apache.velocity.app.event.EventCartridge;
 import org.apache.velocity.runtime.resource.loader.ClasspathResourceLoader;

+import java.io.*;


Lets separate all changes related to formatting to another PR which will be merged first and then we can reduce number of changes only to the important once.
Also be consistent :)

damianszczepanik · 2020-10-14T19:34:07Z

src/test/java/net/masterthought/cucumber/ReportBuilderTest.java

 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.filefilter.WildcardFileFilter;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;

-import net.masterthought.cucumber.generators.OverviewReport;
-import net.masterthought.cucumber.json.Feature;
+import java.io.File;


I like when SDK libs are listed first. By this approach it is not even alphabetically.

damianszczepanik · 2020-10-14T19:35:38Z

src/test/java/net/masterthought/cucumber/ReportBuilderTest.java

@@ -173,22 +166,6 @@ public void copyStaticResources_CopiesRequiredFiles() {
        assertThat(files).hasSize(21);
    }

-    @Test
-    public void createEmbeddingsDirectory_CreatesDirectory() {


in the past I had a problem with this directory - did you remove this test forever or move it to some other class?

FlorianKaemmerer added 4 commits August 13, 2020 13:30

added OutputHandler Interface

77ad5d1

added FilesystemOutputHandler made OutputHandlers configurable adapted AbstractPage to use all configured OutputHandlers

Using OutputHandler to Copy static resource files

074ae7d

Using OutputHandler to store embeddings

9f81cc7

cleanup

2b89940

FlorianKaemmerer mentioned this pull request Aug 13, 2020

[Featurerequest] Enable inmemory Report generation #957

Open

FlorianKaemmerer added 2 commits August 14, 2020 08:26

added some javadoc

33b3f77

added Test

1686f89

removed obsolete Test

added Tests

c91be54

damianszczepanik requested changes Sep 4, 2020

View reviewed changes

FlorianKaemmerer added 6 commits October 5, 2020 10:56

reverted imports

06d3123

updated Tests

added Integration Test for OutputHandler Configuration

df8e4df

reworked integration Test

b24b9b5

reverted import order

3b82c98

added javadoc

5d08c2c

updated asserts

cf2ab79

damianszczepanik reviewed Oct 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add output handler to configuration #958

Add output handler to configuration #958

FlorianKaemmerer commented Aug 13, 2020

codecov-commenter commented Aug 14, 2020 •

edited

Loading

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Oct 14, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Oct 14, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Sep 4, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Sep 4, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 6, 2020

damianszczepanik Sep 4, 2020

damianszczepanik Sep 4, 2020

FlorianKaemmerer Oct 5, 2020

damianszczepanik Oct 14, 2020

damianszczepanik Oct 14, 2020 •

edited

Loading

damianszczepanik Oct 14, 2020

damianszczepanik Oct 14, 2020

Add output handler to configuration #958

Are you sure you want to change the base?

Add output handler to configuration #958

Conversation

FlorianKaemmerer commented Aug 13, 2020

codecov-commenter commented Aug 14, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianszczepanik Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 14, 2020 •

edited

Loading

damianszczepanik Oct 14, 2020 •

edited

Loading