Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trim trailing whitespaces on labels and values #52

Merged
merged 10 commits into from
Jun 13, 2019
Merged

Conversation

vgaidarji
Copy link
Contributor

@vgaidarji vgaidarji commented Jun 11, 2019

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

Update test cases and test more than one CSV file.
Add test case that excludes only the last row.
@codecov
Copy link

codecov bot commented Jun 11, 2019

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 force-pushed the fix-whitespace branch 3 times, most recently from cbec6be to 2c6a1d0 Compare June 11, 2019 19:18
@vgaidarji
Copy link
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
It's needed for test to run on Windows machines too,
can't use sh step.
Remove legacy HudsonTestCase from base SeriesTestCase class
There are no booleans in source XML and test was ignored since beginning.
Test for booleans will be added later if needed.
@vgaidarji
Copy link
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
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
Labels
feature New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants