Skip to content
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.

Sign up for GitHub

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

fix: disable custom security manager for tests which indirectly look up the path of mvn executable #617

Merged
merged 1 commit into from
Nov 15, 2021
Merged

fix: disable custom security manager for tests which indirectly look up the path of mvn executable #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/test/java/sorald/ClasspathModeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ class ClasspathModeTest {
@Test
void resolveClasspathFrom_enablesRepairOfViolation_thatRequiresClasspathToDetect(
@TempDir File workdir) throws IOException {
// use the default security manager
System.setSecurityManager(null);
Comment on lines +22 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

@algomaster99 Just as a note here, there is no "default security manager", really. Setting it to null means there is no security manager, which happens to be the default.

I'd personally also have added a link to the issue detailing why this is necessary. The next person who gets here will have no idea of why we must unset the security manager.

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting it to null means there is no security manager, which happens to be the default.

Setting it to null indirectly means that we have the default SecurityManager enabled, like you said. So are you saying that it is incorrect to not specify the premise and directly write the conclusion?

I'd personally also have added a link to the issue detailing why this is necessary.

I gave a thought about this too. However, I felt that I would have to write that same link at 4 places in the code. Instead, I felt that it would be better if someone just looks up the PR because I have linked the issues properly in the description.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting it to null indirectly means that we have the default SecurityManager enabled, like you said. So are you saying that it is incorrect to not specify the premise and directly write the conclusion?

Well, it's a little bit misleading, because there is no default security manager. The default is to not have a security manager. Saying "use the default security manager" implies that there actually is one. But null is the absence of a value, and null is not a security manager.

Instead, I felt that it would be better if someone just looks up the PR because I have linked the issues properly in the description.

That's great until someone modifies that line and (perhaps changes indentation, moves it, etc) and git blame gives you the wrong answer, and you have to start a bisect to find where the line originated. Perhaps not even in the same file. Sooo, I'd definitely have duplicated that link.

Copy link
Collaborator

@slarse slarse Nov 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For context, a typical Java application does not run with a security manager "unless the application itself defines one": https://docs.oracle.com/javase/tutorial/essential/environment/security.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I will update the comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eh, too much effort imo :). I mean we want to fix this in Spoon, so this should all be removed after that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean we want to fix this in Spoon

Yeah, we won't need this then. :)
I will just create an issue so that at least we don't forget about it.


// arrange
org.apache.commons.io.FileUtils.copyDirectory(
TestHelper.PATH_TO_RESOURCES_FOLDER
Expand Down
6 changes: 6 additions & 0 deletions src/test/java/sorald/MavenLauncherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ public class MavenLauncherTest {
@Test
public void sorald_repairsProductionAndTestCode_inMavenProject(@TempDir File workdir)
throws IOException {
// use the default security manager
System.setSecurityManager(null);

// arrange
org.apache.commons.io.FileUtils.copyDirectory(
TestHelper.PATH_TO_RESOURCES_FOLDER
Expand Down Expand Up @@ -64,6 +67,9 @@ public void sorald_repairsProductionAndTestCode_inMavenProject(@TempDir File wor
@Test
void sorald_repairsRuleViolation_thatRequiresClasspathToDetect(@TempDir File workdir)
throws IOException {
// use the default security manager
System.setSecurityManager(null);

// arrange
org.apache.commons.io.FileUtils.copyDirectory(
TestHelper.PATH_TO_RESOURCES_FOLDER
Expand Down
3 changes: 3 additions & 0 deletions src/test/java/sorald/miner/WarningMinerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,9 @@ public void extractWarnings_statsOutput_containsExpectedAttributes() throws Exce
@Test
void canDetectRuleViolation_thatRequiresClasspath_whenResolvingClasspathInMavenProject(
@TempDir File tempdir) throws Exception {
// use the default security manager
System.setSecurityManager(null);

Path statsFile = tempdir.toPath().resolve("stats.json");
Path projectRoot =
TestHelper.PATH_TO_RESOURCES_FOLDER
Expand Down