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

[algomaster99]

Fixes #612

After a long debugging session with @slarse , we finally decided to disable our custom security manager (SystemExitHandler) as that was causing problems. See this comment for explanation.

[algomaster99]

@slarse This is ready for reviews.

I just had one question. Do we need to restore our custom security manager after execution of the previously failed tests? We did something like this in diffmin when we tried to catch the System.exit() calls.

[khaes-kth]

I guess this is ready to be merged @algomaster99 . Right?

[algomaster99]

@khaes-kth I think so. The patch I have proposed are coherent which @slarse 's explanation so I think we should be good.

[khaes-kth]

Great! I'll merge it. You can always add me/Fernanda/... as a reviewer for the PR when it is ready to be merged.

[algomaster99]

You can always add me/Fernanda/... as a reviewer for the PR

Okay, I will mention you both.

[slarse]

@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.

[algomaster99]

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.

[slarse]

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.

[slarse]

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

[algomaster99]

Right! I will update the comments.

[slarse]

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

[algomaster99]

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.