[WFCORE-5147] Adjusting git integration logging and stability of RemoteSshGitRepositoryTestCase #4352
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uce excessive visibility.
Also remove DEBUG logging from ServerLogger as these should not be translated.
…xception to end boot.
wildfly-ci
added
the
deps-ok
jmesnil
requested changes
Oct 20, 2020
server/src/main/java/org/jboss/as/server/controller/git/ElytronClientCredentialsProvider.java
Show resolved
Hide resolved
server/src/main/java/org/jboss/as/server/controller/git/ElytronClientSshdSessionFactory.java
Show resolved
Hide resolved
| @@ -58,7 +58,7 @@ | |||
| */ | |||
| public class AbstractGitRepositoryTestCase { | |||
|
|
|||
| private final Path jbossServerBaseDir = new File(System.getProperty("jboss.home", System.getenv("JBOSS_HOME"))).toPath().resolve("standalone"); | |||
| private static final Path jbossServerBaseDir = new File(System.getProperty("jboss.home", System.getenv("JBOSS_HOME"))).toPath().resolve("standalone"); | |||
There was a problem hiding this comment.
if that's a constant, we should rename it to JBOSS_SERVER_BASE_DIR
| @AfterClass | ||
| public static void afterClass() throws IOException { | ||
| Security.removeProvider(CREDENTIAL_STORE_PROVIDER.getName()); | ||
| FileUtils.delete(CS_PUBKEY.toFile(), FileUtils.RECURSIVE | FileUtils.RETRY); | ||
| PathUtil.deleteRecursively(backupRoot); |
There was a problem hiding this comment.
should we also restore the configuration in the @AfterClass to avoid keeping the config from the last test for the rest of the test suite?
There was a problem hiding this comment.
The configuration is automatically restored after each individual test on line 260 at the end of the tearDown method.
The @BeforeClass and @afterclass methods are focused on taking a backup once at the start of the test run and deleting that backup at the end of the test run with the @after method handling the individual clean up after every test.
…e errors can be triggered so early in the boot process they are sent to the error output stream meaning the information can not be retrieved from a server log.
|
I think we need to think about how far we want to go with these kinds of review, the majority of this change is related to some opportunities I have seen to improve the logging situation of something that is difficult to debug which could also be a problem if we need to debug this remotely. The issue here is it is not an are of code I am not likely to directly support myself. Two weeks after submitting my PR to clean something up where I have seen some opportunities to improve logging I suddenly receive a lot of comments which are largely cosmetic. Just as I did whilst working on the changes the module definition for the classes with the changed visibility could have been opened to see we already declare all these classes as private API: Regarding the name of an existing variable in the test suite that should have been static all along, sure it is correct it should be in upper case but it is also a private test variable. The discussion about test clean up is worthwhile but it is quick to confirm that the clean up already happens after each test. One point not picked up was the Temp commit that had crept in so I am quickly addressing that now. But overall with PRs such as this I think we really need to ask if the code changes have improved the codebase or introduced a problem, ideally all engineers should be able to quickly submit PRs to clean things up especially around missing logging - but if these come back to you two weeks later so you have to context switch back to them it risks the individual engineer asking the question "Is it worth MY time to clean this up?". Note: This is not the first PR like this, this is just the most recent one where I have stopped my planned work to come back to. |
|
Core - Full Integration Build 10113 outcome was FAILURE using a merge of e9acb69 Failed tests |
|
Retest this please |
jmesnil
approved these changes
Nov 10, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/WFCORE-5147
The RemoteSshGitRepositoryTestCase test case was quite unstable on a resource constrained environment, the test was dependent on monitoring the server.log for an error message but it appears the git integration may log the error in a separate thread so I believe the process can be terminated before the message is logged.
Attempting to debug the issue I realised the test case was not properly restoring the default server configuration between test as attempts to add additional logging categories were being overridden.
Finally we were not logging useful information ourselves, although log and throw can be a bad practice at the moment the uncaught exception only made it's way to System.err - the problem with this was if you are reliant on the server.log the messages would be missing.