-
Notifications
You must be signed in to change notification settings - Fork 160
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
[MENFORCER-390] "requireFilesExist" no longer handles non-canonical paths #297
base: master
Are you sure you want to change the base?
Changes from 14 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. | ||
|
||
|
||
The mounted filesystem(s) will dictate the case-sensitivity rules when checking files. If checks are required that | ||
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 "mounted" and consider getting rid of the parentheses or just changing this to "The filesystem" as is every file is on a single filesystem. 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 |
||
filesystem(s) do not support (e.g. a case-sensitive check on a case-insensitive filesystem) consider adding a custom | ||
plugin that meets the specific needs. | ||
|
||
The following parameters are supported by this rule: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,8 @@ | |
~~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
~~ KIND, either express or implied. See the License for the | ||
~~ specific language governing permissions and limitations | ||
~~ under the License. | ||
~~ under the License. | ||
|
||
------ | ||
Require Files Exist | ||
------ | ||
|
@@ -27,22 +27,25 @@ Require Files Exist | |
|
||
This rule checks that the specified list of files exist. | ||
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 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-sensitivity rules when checking files. If you require checks that your | ||
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: | ||
|
||
* <<message>> - an optional message to the user if the rule fails. | ||
|
||
* <<files>> - A list of files to check. | ||
|
||
* <<allowNulls>> - If null files should be allowed. If allowed, they will be treated as if they do exist. Default is false. | ||
|
||
* <<satisfyAny>> - Allows that one of files can make the rule pass, instead of all the files. Default is false. | ||
|
||
[] | ||
|
||
|
||
Sample Plugin Configuration: | ||
|
||
+---+ | ||
<project> | ||
[...] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,8 @@ | |
~~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
~~ KIND, either express or implied. See the License for the | ||
~~ specific language governing permissions and limitations | ||
~~ under the License. | ||
~~ under the License. | ||
|
||
------ | ||
Require File Size | ||
------ | ||
|
@@ -27,26 +27,29 @@ 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-sensitivity rules when checking files. If you require checks that your | ||
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 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 |
||
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: | ||
|
||
* <<message>> - an optional message to the user if the rule fails. | ||
|
||
* <<files>> - A list of files to check. If this list is empty, the main project artifact will be checked. | ||
|
||
* <<maxsize>> - maximum size in bytes for this file. | ||
|
||
* <<minsize>> - minimum size in bytes for this file. | ||
|
||
* <<allowNulls>> - If null files should be allowed. If allowed, they will be treated as if they do exist. Default is false. | ||
|
||
* <<satisfyAny>> - Allows that one of files can make the rule pass, instead of all the files. Default is false. | ||
|
||
[] | ||
|
||
|
||
Sample Plugin Configuration: | ||
|
||
+---+ | ||
<project> | ||
[...] | ||
|
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; | ||
|
||
|
@@ -29,8 +31,10 @@ | |
|
||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.junit.jupiter.api.Assertions.fail; | ||
import static org.junit.jupiter.api.Assumptions.assumeFalse; | ||
|
||
/** | ||
* Test the "require files don't exist" rule. | ||
|
@@ -105,19 +109,62 @@ void testEmptyFileListAllowNull() { | |
} | ||
|
||
@Test | ||
void testFileDoesNotExist() throws EnforcerRuleException, IOException { | ||
void testDeletedFileDetected() throws EnforcerRuleException, IOException { | ||
File f = File.createTempFile("junit", null, temporaryFolder); | ||
rule.setFilesList(Collections.singletonList(f)); | ||
|
||
// Check the file is detected as being present | ||
EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute); | ||
assumeFalse(e.getMessage() == null); | ||
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 should be an assertNotNull 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. Akready sorted based on earlier comment |
||
|
||
f.delete(); | ||
|
||
assertFalse(f.exists()); | ||
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. 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 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. 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 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. 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 should now pass as the file was deleted | ||
rule.execute(); | ||
} | ||
|
||
@Test | ||
void testSymbolicLinkDeletedDetected() 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)); | ||
// Check the link is detected as being present | ||
EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute); | ||
assumeFalse(e.getMessage() == null); | ||
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. assertNotNull 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 |
||
|
||
linkFile.delete(); | ||
|
||
// Rule should now pass as the target was deleted | ||
rule.execute(); | ||
} | ||
|
||
@Test | ||
void testSymbolicLinkTargetDeletedDetected() throws Exception { | ||
File canonicalFile = File.createTempFile("canonical_", null, temporaryFolder); | ||
File linkFile = Files.createSymbolicLink( | ||
Paths.get(temporaryFolder.getAbsolutePath(), "symbolic.link"), | ||
Paths.get(canonicalFile.getAbsolutePath())) | ||
.toFile(); | ||
|
||
// Check the target is detected as being present | ||
EnforcerRuleException e = assertThrows(EnforcerRuleException.class, rule::execute); | ||
assumeFalse(e.getMessage() == null); | ||
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. assertNotNull 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 |
||
|
||
canonicalFile.delete(); | ||
rule.setFilesList(Collections.singletonList(linkFile)); | ||
|
||
// Rule should now pass as the target was deleted | ||
rule.execute(); | ||
} | ||
|
||
@Test | ||
void testFileDoesNotExistSatisfyAny() throws EnforcerRuleException, IOException { | ||
void testDeletedFileDetectedSatisfyAny() throws EnforcerRuleException, IOException { | ||
File f = File.createTempFile("junit", null, temporaryFolder); | ||
f.delete(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,14 +20,20 @@ | |
|
||
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.*; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.junit.jupiter.api.Assertions.fail; | ||
|
||
/** | ||
* Test the "require files exist" rule. | ||
|
@@ -50,12 +56,34 @@ void testFileExists() throws Exception { | |
} | ||
|
||
@Test | ||
void testFileOsIndependentExists() { | ||
rule.setFilesList(Collections.singletonList(new File("POM.xml"))); | ||
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(); | ||
|
||
EnforcerRuleException e = assertThrows(EnforcerRuleException.class, () -> rule.execute()); | ||
rule.setFilesList(Collections.singletonList(linkFile)); | ||
|
||
assertNotNull(e.getMessage()); | ||
rule.execute(); | ||
} | ||
|
||
@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 have received an exception"); | ||
} catch (Exception e) { | ||
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. 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()); | ||
} | ||
} | ||
|
||
@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.
delete "list of"
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.
Done