Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Conversation

@fo-code
Copy link
Collaborator

@fo-code fo-code commented Aug 7, 2022

Fix for issue #441:

Folders and files which are temporary created while painting the source code are removed after usage now.
I also added an additional assertion to verify that the created temporary directories and only these are deleted.

@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Merging #444 (01d6186) into master (815f1e7) will increase coverage by 0.08%.
The diff coverage is 69.23%.

@@             Coverage Diff              @@
##             master     #444      +/-   ##
============================================
+ Coverage     72.18%   72.27%   +0.08%     
- Complexity      992      997       +5     
============================================
  Files            86       86              
  Lines          3700     3715      +15     
  Branches        433      434       +1     
============================================
+ Hits           2671     2685      +14     
- Misses          883      884       +1     
  Partials        146      146              
Impacted Files Coverage Δ
...age/model/visualization/code/SourceCodeFacade.java 84.05% <69.23%> (-1.02%) ⬇️
.../model/visualization/dashboard/ChangeCoverage.java 100.00% <0.00%> (ø)
...el/visualization/dashboard/CoverageColumnType.java 100.00% <0.00%> (ø)
...l/visualization/dashboard/ChangeCoverageDelta.java 100.00% <0.00%> (ø)
...sualization/dashboard/IndirectCoverageChanges.java 100.00% <0.00%> (ø)
.../model/visualization/dashboard/CoverageColumn.java 78.33% <0.00%> (+5.00%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@fo-code fo-code changed the title Fix for issues #441 Fix for issu #441 Aug 7, 2022
@fo-code fo-code changed the title Fix for issu #441 Fix for issue #441 Aug 7, 2022
Comment on lines 120 to 124
File temporaryDirectory = Paths.get("target", "tmp").toFile();
assertThat(temporaryDirectory.exists()).isTrue();
assertThat(temporaryDirectory.isDirectory()).isTrue();
File[] temporaryFiles = temporaryDirectory.listFiles();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this directory? Does a test case store results there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the directory which is used as temporary directory /tmp within unit tests.
I check this directory in order to verify that painting source code does not store unused files there.
In this case, Files.createTempDirectory(...) uses this.

Nonetheless, I changed this line of code to make it more clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, did you check that the test fails before you applied the fix (the RED during TDD?)? From my understanding, the directories will be part of the Jenkins agent file structure that does use a different directory.

(I think it would be sufficient that the logging statement ist present)

Copy link
Collaborator Author

@fo-code fo-code Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I did. I made the tests fail, then I inserted the new code and it worked. Also, I debugged it and even checked the directories while doing this to make sure these temporary folders are created exacly there.
log

Also as I understand it, Files.createTempDirectory(...) points to System.getProperty("java.io.tmpdir") by default, if it is not overwritten using a unit test rule e.g. - also see here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. All tests use the controller to execute the builds. (Only the docker based tests would use a temporary folder in the docker container).

@uhafner uhafner added the bug Bugs or performance problems label Aug 7, 2022
@uhafner uhafner changed the title Fix for issue #441 [#441] Delete temporary folders on agent after coloring source code files Aug 7, 2022
@uhafner uhafner merged commit 163b080 into jenkinsci:master Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Bugs or performance problems

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants