FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix CompressArchiveUtil unit tests by hugares · Pull Request #1211 · docker-java/docker-java · GitHub

Repository navigation

Fix CompressArchiveUtil unit tests - #1211

Merged
KostyaSha merged 1 commit into
docker-java:masterfrom
hugares:fix-CompressArchiveUtilTest
Jul 11, 2019
Merged

KostyaSha merged 1 commit into
docker-java:masterfrom
hugares:fix-CompressArchiveUtilTest

Conversation

hugares commented Jul 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

The tar method's contract is very clear, it creates a tar archive from
files in a folder. The unit tests should validate that the tar archive
was created and that the content is what it is supposed to be. Same
apply to the archiveTARFiles method.

Before, in order to validate that the tar archive was created, the unit
tests were un-archiving the tar. This is wrong as you end up implicitly
testing the extraction code which could be faulty.

In addition, the 2 tests for symlinks were not even checking if the
extracted file was a symlink, they were only checking if the file or
folder were readable.

Rewrite the tests to assert directly the content of the archive. Also
fix the 2 symlinks tests to make sure the tar preserve them. Fixing the
unit tests exposed that CompressArchiveUtil methods do not preserve the
symlinks which is a bug. Add @ignore annotation for those tests until
the bug is fixed.

This change is a preparation change to fix the archive creation to
preserve symlinks so add tests to increase archive creation code
coverage close to 100%. This will allow to fix the code while making
sure no regression are introduced.

Related to issue #532


This change is 

hugares force-pushed the fix-CompressArchiveUtilTest branch from 3a0176e to 6c2a060 Compare July 8, 2019 17:36

codecov-io commented Jul 9, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1211 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1211      +/-   ##
==========================================
+ Coverage      58%   58.03%   +0.02%     
==========================================
  Files         455      455              
  Lines        8835     8835              
  Branches      533      533              
==========================================
+ Hits         5125     5127       +2     
  Misses       3431     3431              
+ Partials      279      277       -2
Impacted Files Coverage Δ
...thub/dockerjava/core/util/CompressArchiveUtil.java 93.02% <0%> (+4.65%) ⬆️

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 1f1eab6...1f11a5a. Read the comment docs.

hugares force-pushed the fix-CompressArchiveUtilTest branch 3 times, most recently from 37426ee to e143e1d Compare July 9, 2019 19:59

hugares commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

@KostyaSha This pull request is ready to review. Once it is merged, I will be able to fix and open a MR to fix issue with symlink[1]

[1]#532

hugares force-pushed the fix-CompressArchiveUtilTest branch from e143e1d to 1f11a5a Compare July 10, 2019 02:10

hugares commented Jul 10, 2019

Copy link
Copy Markdown
Contributor Author

In addition to this PR, I have one commit ready which removes the duplicated code (3 times) to create an entry in an archive and one commit fixing the symlink issue (#532).

Do you prefer that I I close this PR and open one with all 3 commits or we keep this one open and wait until it is merged to open 2nd PR and so on for the 3rd PR?

KostyaSha added this to the 3.1.3 milestone Jul 10, 2019

Copy link
Copy Markdown
Member

needs rebase now

The tar method's contract is very clear, it creates a tar archive from
files in a folder. The unit tests should validate that the tar archive
was created and that the content is what it is supposed to be. Same
apply to the archiveTARFiles method.

Before, in order to validate that the tar archive was created, the unit
tests were un-archiving the tar. This is wrong as you end up implicitly
testing the extraction code which could be faulty.

In addition, the 2 tests for symlinks were not even checking if the
extracted file was a symlink, they were only checking if the file or
folder were readable.

Rewrite the tests to assert directly the content of the archive. Also
fix the 2 symlinks tests to make sure the tar preserve them. Fixing the
unit tests exposed that CompressArchiveUtil  methods do not preserve the
symlinks which is a bug. Add @ignore annotation for those tests until
the bug is fixed.

This change is a preparation change to fix the archive creation to
preserve symlinks so add tests to increase archive creation code
coverage close to 100%. This will allow to fix the code while making
sure no regression are introduced.

Related to issue docker-java#532
hugares force-pushed the fix-CompressArchiveUtilTest branch from 1f11a5a to 356c209 Compare July 10, 2019 20:05

hugares commented Jul 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

needs rebase now

Done

KostyaSha merged commit 098f74d into docker-java:master Jul 11, 2019

Copy link
Copy Markdown
Member

Thanks

hugares deleted the fix-CompressArchiveUtilTest branch July 19, 2019 11:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL