-
Notifications
You must be signed in to change notification settings - Fork 176
[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297
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
base: master
Are you sure you want to change the base?
Changes from 7 commits
4568135
c7ca861
81b8027
0b45664
326ff7f
b82306d
01570d4
e6cd78c
11dbb95
e0d8be0
e6baf8f
14449e1
b164816
61211d9
4c62cb6
86dc7ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,10 @@ | |
| Require Files Don't Exist | ||
|
|
||
| This rule checks that the specified list of files do not exist. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete "list of"
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
|
|
||
| The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. case-sensitivity
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
| filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete "then you should"
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
| adding your own custom plugin that meets your specific needs. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. your own --> a
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
|
|
||
| The following parameters are supported by this rule: | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,10 @@ | |
| Require Files Exist | ||
|
|
||
| This rule checks that the specified list of files exist. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
|
|
||
| The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. case-sensitivity
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Apols. |
||
| filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider | ||
| adding your own custom plugin that meets your specific needs. | ||
|
|
||
| The following parameters are supported by this rule: | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,10 @@ | |
| Require File Size | ||
|
|
||
| This rule checks that the specified list of files exist and are within the specified size range. | ||
|
|
||
|
|
||
| The mounted filesystem(s) will dictate the case-sensitive rules when checking files. If you require checks that your | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. case-sensitivity
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Apols. |
||
| filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) then you should consider | ||
| adding your own custom plugin that meets your specific needs. | ||
|
|
||
| The following parameters are supported by this rule: | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,17 +20,16 @@ | |
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
|
|
||
| import org.apache.maven.enforcer.rule.api.EnforcerRuleException; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.fail; | ||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. project style does not use wildcard imports
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated, and in |
||
|
|
||
| /** | ||
| * Test the "require files don't exist" rule. | ||
|
|
@@ -107,15 +106,58 @@ void testEmptyFileListAllowNull() { | |
| @Test | ||
| void testFileDoesNotExist() throws EnforcerRuleException, IOException { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this test. It checks that we create a file, delete it, and then that the file doesn't exist? Consider renamingi
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to just be a minor variation on the original test code, but I can change the name. There is also "testFileDoesNotExistSatisfyAny", which is original code.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, renaming is a good idea
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please give me a format of the new method name you'd prefer and I will ensure that all To be clear, I disagree with this but it is not my project and so I will follow your direction.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't change other methods not related to this PR. keep the PR focused.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But that means this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better 9 wrong and 1 right than 10 wrong. That's the fundamental principle.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should the new names be?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. testSymbolicLinkDeletedDetected (spelling) but otherwise sounds good
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| File f = File.createTempFile("junit", null, temporaryFolder); | ||
| rule.setFilesList(Collections.singletonList(f)); | ||
|
|
||
| EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute); | ||
| assertNotNull(e.getMessage()); | ||
|
|
||
| f.delete(); | ||
|
|
||
| assertFalse(f.exists()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think my confusion is that this is the only assert. It should be changed to an assumeFalse or removed.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a quick negative check to ensure the file is detected and the test does not pass by fluke. It is consistent with checks in the other tests (throws an exception with a message). And I am sorry, but I do no understand how I can see a mix of checkin/not-checking for an unexpected exception, I can add and explicit check for one and add a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test setup failure does not mean the test failed. It means the test wasn't run. That's the difference between assume and assert. JUnit reports these two cases differently.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The wrong one was changed. Line 122 should be changed or better yet deleted. The point of the test is not whether f.delete() succeeds. You're not testing the delete method in java.io.File.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
|
||
| rule.setFilesList(Collections.singletonList(f)); | ||
|
|
||
| rule.execute(); | ||
| } | ||
|
|
||
| @Test | ||
| void testSymbolicLinkDoesNotExist() throws Exception { | ||
| File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder); | ||
| File linkFile = Files.createSymbolicLink( | ||
| Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"), | ||
| Paths.get(canonicalFile.getAbsolutePath())) | ||
| .toFile(); | ||
|
|
||
| try { | ||
| rule.setFilesList(Collections.singletonList(linkFile)); | ||
| EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute); | ||
| assertNotNull(e.getMessage()); | ||
|
|
||
| linkFile.delete(); | ||
| rule.execute(); | ||
| } finally { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use try with resources for this?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don;'t think so, isn't that just for autoclosables?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at other tests, it does not seem to be project standard to ensure tests delete files they create; I could just remove the try block entirely in that case.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the temporary folder rule should take care of that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK |
||
| if (linkFile.exists()) { | ||
| linkFile.delete(); | ||
| } | ||
| canonicalFile.delete(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void testSymbolicLinkTargetDoesNotExist() throws Exception { | ||
| File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder); | ||
| File linkFile = Files.createSymbolicLink( | ||
| Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"), | ||
| Paths.get(canonicalFile.getAbsolutePath())) | ||
| .toFile(); | ||
| canonicalFile.delete(); | ||
| rule.setFilesList(Collections.singletonList(linkFile)); | ||
|
|
||
| try { | ||
| rule.execute(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no exception is a pass?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. This whole class it basically the inverse of |
||
| } finally { | ||
| linkFile.delete(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void testFileDoesNotExistSatisfyAny() throws EnforcerRuleException, IOException { | ||
| File f = File.createTempFile("junit", null, temporaryFolder); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,8 @@ | |
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Paths; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
|
|
||
|
|
@@ -50,12 +52,40 @@ void testFileExists() throws Exception { | |
| } | ||
|
|
||
| @Test | ||
| void testFileOsIndependentExists() { | ||
| rule.setFilesList(Collections.singletonList(new File("POM.xml"))); | ||
|
|
||
| EnforcerRuleException e = assertThrows(EnforcerRuleException.class, () -> rule.execute()); | ||
| void testSymbolicLinkExists() throws Exception { | ||
| File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder); | ||
| File linkFile = Files.createSymbolicLink( | ||
| Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"), | ||
| Paths.get(canonicalFile.getAbsolutePath())) | ||
| .toFile(); | ||
| rule.setFilesList(Collections.singletonList(linkFile)); | ||
|
|
||
| try { | ||
| rule.execute(); | ||
| } finally { | ||
| linkFile.delete(); | ||
| canonicalFile.delete(); | ||
| } | ||
| } | ||
|
|
||
| assertNotNull(e.getMessage()); | ||
| @Test | ||
| void testSymbolicLinkTargetDoesNotExist() throws Exception { | ||
| File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder); | ||
| File linkFile = Files.createSymbolicLink( | ||
| Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"), | ||
| Paths.get(canonicalFile.getAbsolutePath())) | ||
| .toFile(); | ||
| canonicalFile.delete(); | ||
| rule.setFilesList(Collections.singletonList(linkFile)); | ||
|
|
||
| try { | ||
| rule.execute(); | ||
| fail("Should get an exception"); | ||
| } catch (Exception e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which exception should be thrown here? Be specific.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| assertNotNull(e.getMessage()); | ||
| } finally { | ||
| linkFile.delete(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO
return file == null || file.exists()
is clearer, but up to you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old habits die hard. :-)