Skip to content

Trim trailing whitespaces on labels and values #52

Merged
vgaidarji merged 10 commits into
masterfrom
fix-whitespace
Jun 13, 2019
Merged

Trim trailing whitespaces on labels and values #52
vgaidarji merged 10 commits into
masterfrom
fix-whitespace

Conversation

@vgaidarji

@vgaidarji vgaidarji commented Jun 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a follow up of #45.

What has been done

  1. Used trim() on labels and y values
  2. Updated tests
  3. Added windows node to Jenkins builds
  4. Clean up tests and increase test execution time by migration all tests to org.junit and removal of legacy HudsonTestCase from base SeriesTestCase class.

In Windows, if you are trying to create a csv file using echo, you will get trailing whitespace at the end of each line. For example:

echo label1,label2,label3 > output.csv
echo 1,2,3 >> output.csv

output.csv will now contain a trailing space at the end of each line. Now if the user says they want to plot only "label3" (using exclusion/inclusion criteria), the parser will not find a match, because it has captured "label3 " (with a space) as the label for the row.

Screenshots

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

2018-03-25 17_25_01-csvseries java - plot-plugin - visual studio code
2018-03-25 17_27_10-csvseries java - plot-plugin - visual studio code

How to test

  1. 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%
  1. 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

@skelliam @vgaidarji
Trim trailing whitespaces on labels and values
e7ddcac 
Update test cases and test more than one CSV file.
Add test case that excludes only the last row.
@codecov

codecov Bot commented Jun 11, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #52 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #52      +/-   ##
============================================
- Coverage     42.06%   41.89%   -0.17%     
+ Complexity      178      176       -2     
============================================
  Files            18       18              
  Lines          1222     1222              
  Branches        186      186              
============================================
- Hits            514      512       -2     
- Misses          644      646       +2     
  Partials         64       64
Impacted Files Coverage Δ Complexity Δ
src/main/java/hudson/plugins/plot/CSVSeries.java 49.6% <100%> (-0.79%) 19 <0> (-1)
src/main/java/hudson/plugins/plot/XMLSeries.java 71.32% <0%> (-0.7%) 35% <0%> (-1%)

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 a88cfe6...29898bd. Read the comment docs.

@vgaidarji vgaidarji changed the title Fix whitespace Trim trailing whitespaces on labels and values Jun 11, 2019
@vgaidarji vgaidarji mentioned this pull request Jun 11, 2019
Closed
5 tasks
@vgaidarji
vgaidarji force-pushed the fix-whitespace branch 3 times, most recently from cbec6be to 2c6a1d0 Compare June 11, 2019 19:18
@vgaidarji

vgaidarji commented Jun 11, 2019

Copy link
Copy Markdown
Contributor Author

Need to fix tests on Windows node in Jenkins (most probably caused by sh step in pipeline under test):

[2019-06-11T19:20:24.813Z] [ERROR] Tests run: 16, Failures: 15, Errors: 0, Skipped: 0, Time elapsed: 29.378 s <<< FAILURE! - in hudson.plugins.plot.PlotBuilderTest
[2019-06-11T19:20:24.813Z] [ERROR] testWithProperties(hudson.plugins.plot.PlotBuilderTest)  Time elapsed: 1.645 s  <<< FAILURE!
[2019-06-11T19:20:24.813Z] java.lang.AssertionError: 
[2019-06-11T19:20:24.813Z] unexpected build status; build log was:
[2019-06-11T19:20:24.813Z] ------
[2019-06-11T19:20:24.813Z] Started
[2019-06-11T19:20:24.813Z] [Pipeline] node
[2019-06-11T19:20:24.813Z] Running on master in C:\windows\TEMP\jenkinsTests.tmp\jenkins2289443509074702808test\workspace\projectUnderTest
[2019-06-11T19:20:24.813Z] [Pipeline] {
[2019-06-11T19:20:24.813Z] [Pipeline] sh
[2019-06-11T19:20:24.813Z] [C:\windows\TEMP\jenkinsTests.tmp\jenkins2289443509074702808test\workspace\projectUnderTest] Running shell script
[2019-06-11T19:20:24.813Z] [Pipeline] }
[2019-06-11T19:20:24.813Z] [Pipeline] // node
[2019-06-11T19:20:24.813Z] [Pipeline] End of Pipeline
[2019-06-11T19:20:24.813Z] java.io.IOException: Cannot run program "nohup" (in directory "C:\windows\TEMP\jenkinsTests.tmp\jenkins2289443509074702808test\workspace\projectUnderTest"): CreateProcess error=2, The system cannot find the file specified

@vgaidarji vgaidarji self-assigned this Jun 12, 2019
vgaidarji added 7 commits June 12, 2019 09:15
@vgaidarji
Switch to writeFile in test pipelines
dfcc52a 
It's needed for test to run on Windows machines too,
can't use sh step.
@vgaidarji
Reformat introduced test changes
7e32045
@vgaidarji
Remove unused constant
a96a245
@vgaidarji
Use proper assertions and fail methods in tests
46f5139
@vgaidarji
Simplify workspace directory creation for tests
51a7a4a
@vgaidarji
Migrate all tests to org.junit
fb575ef 
Remove legacy HudsonTestCase from base SeriesTestCase class
@vgaidarji
Remove XML Boolean test
2053b48 
There are no booleans in source XML and test was ignored since beginning.
Test for booleans will be added later if needed.
@vgaidarji

vgaidarji commented Jun 13, 2019

Copy link
Copy Markdown
Contributor Author

We can improve tests even more, but refactoring done in this PR already makes them cleaner and better organized.

Changes introduced for trimming are covered with tests and shouldn't bring any issues.

@skelliam I'm going to merge & release these changes soon 🎉 :shipit:

@vgaidarji
vgaidarji merged commit adc2375 into master Jun 13, 2019
@vgaidarji
vgaidarji deleted the fix-whitespace branch June 13, 2019 19:56
@vgaidarji vgaidarji mentioned this pull request Jun 13, 2019
Merged
5 tasks
@vgaidarji vgaidarji added the feature New feature or improvement label Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@vgaidarji vgaidarji

Labels

feature New feature or improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vgaidarji @skelliam