[JENKINS-73714] Fix filtering of icons in jar file

[strangelookingnerd]

See https://issues.jenkins.io/browse/JENKINS-73714 for more details.

During the refactoring in 1b41824 the collection of the icons was broken in case they are read from the JAR file due to the FileSystem being closed too early.

Testing done

I did not find a way to reproduce the issue with a regular test without manually manipulating the classpath. However the manual tests showed that these changes fix the issue at hand.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 71.15%. Comparing base (ab9790e) to head (f6a3c13).
Report is 86 commits behind head on main.

Files with missing lines	Patch %	Lines
.../jenkins/plugins/fontawesome/FontAwesomeIcons.java	71.42%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main     #283       +/-   ##
=============================================
+ Coverage     50.00%   71.15%   +21.15%     
- Complexity       17       19        +2     
=============================================
  Files             3        2        -1     
  Lines            76       52       -24     
  Branches          5        3        -2     
=============================================
- Hits             38       37        -1     
+ Misses           31        9       -22     
+ Partials          7        6        -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[uhafner]

Thanks! Maybe it makes sense to write a UI test so we can remove the non-jar code and see such bugs during development.

[strangelookingnerd]

Thanks! Maybe it makes sense to write a UI test so we can remove the non-jar code and see such bugs during development.

I’ll try to look into it and may create a PR if I find a solution. But just to be sure: The non-JAR path is only relevant during development but will not be used in „production“ usage. There is no situation where the plugins are being unpacked in a Jenkins deployment?

[uhafner]

Thanks! Maybe it makes sense to write a UI test so we can remove the non-jar code and see such bugs during development.

I’ll try to look into it and may create a PR if I find a solution.

I'm not sure if it is worth the effort. This requires a lot of changes in the plugin structure. Do you have a UI test in your consumer plugin?

But just to be sure: The non-JAR path is only relevant during development but will not be used in „production“ usage. There is no situation where the plugins are being unpacked in a Jenkins deployment?

I don't know.

[strangelookingnerd]

Thanks! Maybe it makes sense to write a UI test so we can remove the non-jar code and see such bugs during development.

I’ll try to look into it and may create a PR if I find a solution.

I'm not sure if it is worth the effort. This requires a lot of changes in the plugin structure. Do you have a UI test in your consumer plugin?

I gave it a go yesterday and did not really come up with something that actually works.

Using RealJenkinsRule has more or less the same classpath issue from I can tell.
It would work when running Jenkins in a container and have a completely disjunct classpath from what ever the IDE / CI is providing. The overhead seems unreasonable to me.

Adding tests to my consumer plugin seems way more reasonable.

[uhafner]

Thanks! Maybe it makes sense to write a UI test so we can remove the non-jar code and see such bugs during development.

I’ll try to look into it and may create a PR if I find a solution.

I'm not sure if it is worth the effort. This requires a lot of changes in the plugin structure. Do you have a UI test in your consumer plugin?

I gave it a go yesterday and did not really come up with something that actually works.

Using RealJenkinsRule has more or less the same classpath issue from I can tell.

Yes, all integration tests run with the classes folder as classpath.

It would work when running Jenkins in a container and have a completely disjunct classpath from what ever the IDE / CI is providing. The overhead seems unreasonable to me.

Adding tests to my consumer plugin seems way more reasonable.

Yes, this is exactly what I mean: we need a UI test (with ATH) to test this setup. This requires a new plugin structure, see https://github.com/jenkinsci/git-forensics-plugin/blob/main/ui-tests/src/test/java/io/jenkins/plugins/forensics/git/SmokeTests.java or https://github.com/jenkinsci/warnings-ng-plugin/blob/main/ui-tests/src/test/java/io/jenkins/plugins/analysis/warnings/SmokeTests.java for some examples.