-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Tidy up executor UTs #11249
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?
Tidy up executor UTs #11249
Conversation
As they seem problematic.
cwd, | ||
userHome, | ||
List.of("eu.maveniverse.maven.plugins:toolbox:0.7.4:help"), | ||
List.of("eu.maveniverse.maven.plugins:toolbox:0.13.7:help"), |
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.
Is there a way to have the version externalised in the POM so that it can be upgraded more easily ?
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.
Technically, these changes are not needed, are not mandatory. But I did this merely "to align" all toolbox versions used in Maven build, to not waste time on pulling various different versions of it over and over again.
Will see if we can somehow centralize the config/GAV for it.
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.
Technically, these changes are not needed, are not mandatory. But I did this merely "to align" all toolbox versions used in Maven build, to not waste time on pulling various different versions of it over and over again.
Yes, and my remark goes into the exact same direction ;-). IF we can align things and have a single place of configuration, that would be even better!
Files.createDirectories(userHome); | ||
MimirInfuser.infuseUW(userHome); | ||
|
||
System.out.println("=== " + testInfo.getTestMethod().orElseThrow().getName()); |
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.
I think we want to remove those System.out.println
before merging...
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.
Or use proper logging...
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.
While in general I completely agree about this stance, in this very case I'd leave them; their reason of existence is really to easy our work of "figuring out what happens". As all these tests are testing "execution of maven to provide us some information" (with all the play of force stdout plus quiet mode). As long as we cannot "configure output to terminal and file differently", I think it is simplest and "cheapest" to leave them. The reason I did not want logger in test CP, is to keep CP classpath "as pristine as possible", as we have SLF4J logging already present JVM, in the "embedded Maven".
So this is my 5 cents, I can remove all this if you insist, but again, the point is to have them in surefire grabbed stdout for easier debugging, thats all. I'd not remove them, nor would use "fully fledged logging framework" either, but without them we are left in dark (as maven cannot write to stdout one thing, while logging another to file).
As it is create twice: once in unforked JVM by Junit via TestSuiteOrdering and once in forked JVM, but the system property is set only in latter. So use default, as class orderer does not use anything related to toolbox.
As they seem problematic.