Use trim() to remove trailing whitespace before using values from csv by skelliam · Pull Request #45 · jenkinsci/plot-plugin

skelliam · 2018-03-25T22:23:17Z

Use trim() on label values
Update test cases and test more than one csv file
Add test case that excludes only the last row

What has been done

Use trim() on labels and y values
Update test cases

Screenshots

Debugging in Visual Studio code, I took screenshots of the original code grabbing the trailing space from the new test case I added.

How to test

If someone wants to reproduce this on Windows, I used this as a Windows batch script:

set plotfile=test.csv

IF NOT EXIST %plotfile% (
   echo foo,bar,thing > %plotfile%
)

echo %time:~-5%,7,%time:~-2% >> %plotfile%

Now try to plot only the last column named "thing" using the plot plugin. It will not get plotted because the plugin only discovers the name "thing " (with space).

Checklist

Git commits follow best practices
Build passes in Jenkins
Appropriate tests or explanation to why this change has no tests
JIRA issue is well described (problem explanation, steps to reproduce, screenshots)
For dependency updates: links to external changelogs and, if possible, full diffs

Use trim() on label values Update test cases and test more than one csv file Add test case that excludes only the last row

skelliam · 2018-03-25T22:42:13Z

-        // first create a FilePath to load the test Properties file.
-        File workspaceDirFile = new File("target/test-classes/");
-        FilePath workspaceRootDir = new FilePath(workspaceDirFile);
+        for (int testfilenum = 0; testfilenum < FILES.length; testfilenum++) {


Here I wrapped the test case with a for loop to test more than one csv file.

skelliam · 2018-03-25T22:43:06Z

-        CSVSeries series = new CSVSeries(FILES[0], null, null, null, true);
-        assertTrue(series.getDisplayTableFlag());
+        CSVSeries series;
+        for (int testfilenum = 0; testfilenum < FILES.length; testfilenum++) {


again wrap existing test case with for loop to test multiple files

skelliam · 2018-03-25T22:43:21Z

-        // first create a FilePath to load the test Properties file.
-        File workspaceDirFile = new File("target/test-classes/");
-        FilePath workspaceRootDir = new FilePath(workspaceDirFile);
+        for (int testfilenum = 0; testfilenum < FILES.length; testfilenum++) {


test multiple files again

skelliam · 2018-03-25T22:43:57Z


-        // Create a new CSV series.
-        CSVSeries series = new CSVSeries(FILES[0], "http://localhost:8080/%name%/%index%/", "OFF", "", false);
+    public void testCSVSeriesIncludeOnlyLastColumn() {


new test case, include only the last column by name

skelliam · 2018-03-25T22:44:37Z

@@ -0,0 +1,3 @@
+foo,bar,thing 
+07.81,7,81 
+25.42,7,42 


CSV file from Windows echo -- there is an extra space (0x20) character at the end of each line that messes up the plot plugin.

skelliam · 2018-03-25T23:04:55Z

Photo of before/after:

vgaidarji

It doesn't break plotting for me on Mac OS (don't have a Windows machine to check), and plots constructing for me even without this change.

Thanks for adding tests, those are really helpful.
Few tweaks are still required. Other than that everything is great.

vgaidarji · 2018-03-30T07:56:34Z

+                    ++lineNum;
                }
-            } catch (IOException e) {
+                assertTrue("Line count is not equal " + lineNum + " expected " + LINES[testfilenum], LINES[testfilenum] == lineNum);


For some reason, Checkstyle line length rule isn't enforced in tests. I need to fix that.

But if you can reformat the code according to max line length = 100 rule, that would be awesome.

@vgaidarji For testing on MacOS, you only need to make sure that a trailing space is added at the end of the last label and the last value in each row. "echo" in MacOS or Linux probably does not have the same effect. The other thing you could do is remove the trim() statements I added in the functional code, but keep the tests the same. Then run the tests and see if they fail on MacOS.

@skelliam Yes, I did exactly that and it works for me even before trim() was added.
I have added spaces after the last label and after each last value in each line, and then selected that label in plot configuration and specified it without trailing space. Plots rendered just fine.

@vgaidarji, maybe it has something to do with the CR LF endings in Windows? If you don't implement trim(), do the new tests pass? If so that is really strange...

I would really like to somehow have another user verify this -- I guess it is pretty low risk though.

@skelliam yes, it's a very low risk. Trimming trailing spaces sounds very reasonable to me.
I've verified following cases:

plot plugin build from 1ccbcc8

"column" & "value" & include column by name "column" = plots are shown

"column " & "value " & include column by name "column" = plots are shown

"column " & "value " & include column by name "column " = plots are not shown

"column" & "value" & include column by name "column " = plots are not shown

Plot plugin 2.0.3 version

"column" & "value" & include column by name "column" = plots are shown

"column " & "value " & include column by name "column" = plots are shown

"column " & "value " & include column by name "column " = plots are not shown

"column" & "value" & include column by name "column " = plots are not shown

As you can see, the behavior is the same. I'm on Mac OS High Sierra 10.13.3.

I'm just wondering why I can't reproduce the issue you described and if it's OS related:

If someone wants to reproduce this on Windows, I used this as a Windows batch

set plotfile=test.csv IF NOT EXIST %plotfile% ( echo foo,bar,thing > %plotfile% ) echo %time:~-5%,7,%time:~-2% >> %plotfile%

Now try to plot only the last column named "thing" using the plot plugin. It will not get plotted because the plugin only discovers the name "thing " (with space).

As I'm not getting plots when column name has the trailing space.

"column " & "value " & include column by name "column " = plots are not shown

@skelliam Could you please confirm if you have the same behavior?

Also you can execute project tests on Windows machine by modifying the Jenkinsfile:

buildPlugin(platforms: ['linux', 'windows'])

or

buildPlugin() // 'linux', 'windows' by default

To be honest, I'm not sure if they pass or fail, never tried on this project and we used 'linux' when Jenkinsfile was added in #31.

vgaidarji · 2018-03-30T07:58:58Z


-    private static final String[] FILES = {"test.csv"};
+    private static final String[] FILES = {"test.csv", "spacestest.csv", "test_trailing_semicolon.csv"};
+    private static final int[] LINES = {2, 3, 2};  //lines in the file including header


Comments should start with a space like // lines in ....
For some reason, this rule is also not enforced in tests.

vgaidarji · 2018-03-30T07:59:37Z

-        CSVSeries series = new CSVSeries(FILES[0], null, null, null, true);
-        assertTrue(series.getDisplayTableFlag());
+        CSVSeries series;
+        for (int testfilenum = 0; testfilenum < FILES.length; testfilenum++) {


Please follow camelCase in variables naming.
-> testFileNumber.

vgaidarji · 2018-03-30T08:05:44Z

+    private static final int[] CORRECTED_COLS = {8, 3, 8};  //corrected for the trailing comma case
+    private static final int[] TOTAL_POINTS = {8, 6, 8};  //total data points in the file
+    private static final String[] LAST_COLUMN_NAME = {"error %", "thing", "error %"};  //the label on the last column
+    private static final String[] LAST_POINT = {"0.37", "42", "0.37"};  //the value of the last data point


Please remove unused constant.

vgaidarji · 2018-03-30T09:05:35Z

-        File workspaceDirFile = new File("target/test-classes/");
-        FilePath workspaceRootDir = new FilePath(workspaceDirFile);
+        for (int testfilenum = 0; testfilenum < FILES.length; testfilenum++) {
+            // first create a FilePath to load the test Properties file.


Let's extract following code block into a separate method and reuse in tests:

// first create a FilePath to load the test Properties file. File workspaceDirFile = new File("target/test-classes/"); FilePath workspaceRootDir = new FilePath(workspaceDirFile); LOGGER.info("workspace File path: " + workspaceDirFile.getAbsolutePath()); LOGGER.info("workspace Dir path: " + workspaceRootDir.getName());

vgaidarji · 2018-03-30T09:12:25Z

+            LOGGER.info("Got " + points.size() + " plot points");
+            testPlotPoints(points, TOTAL_POINTS[testfilenum]);
+
+            int pointnum = 0;


Please follow camelCase in variables naming.
-> pointNumber
-> columnNumber

vgaidarji · 2018-05-08T11:02:13Z

@skelliam hey, are you still around?

skelliam · 2018-05-11T15:51:28Z

@vgaidarji Hey I'm still around, just haven't had much of a chance to get to this... Seems like trim() is a low risk addition though, and you found other cases where you could use trim() i.e. on the column name to include?

vgaidarji · 2018-05-12T05:50:42Z

@skelliam yes, let's fix the issues pointed above, try running tests on Windows slave (#45 (comment)), and we can merge this one.

vgaidarji · 2018-08-02T13:07:35Z

@skelliam are you still around? ^^

skelliam · 2018-09-01T20:31:54Z

Holiday weekend here in the US. Maybe I can get to this this weekend? Sorry for no feedback.

codecov · 2018-09-20T15:06:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@658edf1). Click here to learn what that means.
The diff coverage is 41.86%.

@@            Coverage Diff            @@
##             master      #45   +/-   ##
=========================================
  Coverage          ?   42.06%           
  Complexity        ?      178           
=========================================
  Files             ?       18           
  Lines             ?     1222           
  Branches          ?      186           
=========================================
  Hits              ?      514           
  Misses            ?      644           
  Partials          ?       64

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/hudson/plugins/plot/PlotReport.java	`47% <0%> (ø)`	`10 <0> (?)`
src/main/java/hudson/plugins/plot/Plot.java	`25.75% <0%> (ø)`	`30 <0> (?)`
src/main/java/hudson/plugins/plot/PlotBuilder.java	`62.16% <58.06%> (ø)`	`22 <7> (?)`

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 658edf1...1ccbcc8. Read the comment docs.

vgaidarji · 2019-01-03T17:48:40Z

@skelliam same question 🙂

are you still around? ^^

vgaidarji · 2019-06-11T18:45:14Z

@skelliam I'll try to address remaining issues myself in #52. Didn't want to just abandon your work.
Thank you for your contribution 🎉

skelliam · 2019-06-14T11:20:29Z

Thanks @vgaidarji, sorry to have abandoned this, I made the changes at work and since then haven't been close to our Jenkins build. 🙏

vgaidarji · 2019-06-14T11:57:44Z

@skelliam no problem, it's understandable 😉 Thanks for your contribution 💪
New version 2.1.1 released and included into plugin wiki https://wiki.jenkins.io/display/JENKINS/Plot+Plugin.

Use trim() to remove trailing whitespace before using the value

1ccbcc8

Use trim() on label values Update test cases and test more than one csv file Add test case that excludes only the last row

skelliam commented Mar 25, 2018

View reviewed changes

vgaidarji self-assigned this Mar 30, 2018

vgaidarji self-requested a review March 30, 2018 06:35

vgaidarji reviewed Mar 30, 2018

View reviewed changes

vgaidarji closed this Jun 11, 2019

vgaidarji mentioned this pull request Jun 13, 2019

Trim trailing whitespaces on labels and values #52

Merged

5 tasks

Uh oh!

Uh oh!

Conversation

skelliam commented Mar 25, 2018

What has been done

Screenshots

How to test

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skelliam commented Mar 25, 2018

Uh oh!

vgaidarji left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vgaidarji commented May 8, 2018

Uh oh!

skelliam commented May 11, 2018

Uh oh!

vgaidarji commented May 12, 2018

Uh oh!

vgaidarji commented Aug 2, 2018

Uh oh!

skelliam commented Sep 1, 2018

Uh oh!

codecov Bot commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vgaidarji commented Jan 3, 2019

Uh oh!

vgaidarji commented Jun 11, 2019

Uh oh!

skelliam commented Jun 14, 2019

Uh oh!

vgaidarji commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

vgaidarji left a comment •

edited

Loading

codecov Bot commented Sep 20, 2018 •

edited

Loading

vgaidarji commented Jun 14, 2019 •

edited

Loading