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

Reducing console output during build #39

Merged
merged 5 commits into from Oct 24, 2017

Conversation

Projects
None yet
2 participants
@venushka
Contributor

venushka commented Oct 20, 2017

Fix for #38

Also removed sudo flag from travis build configuration as it pins the builds to out-dated version of linux that travis are phasing out.

venushka added some commits Oct 20, 2017

Switching off sudo so Travis can now use virtualised environments wit…
…hout having to use ubuntu precise

Redirecting all test logs to target/ directory as they make the build output noisy
Making the log4j.properties consistent across the modules
Removing the redundant logger entry setting whole of org.alfasoftware.morf to INFO as the root logger is set to INFO
@venushka

This comment has been minimized.

Show comment
Hide comment
@venushka

venushka Oct 20, 2017

Contributor

Already useful, there are some JavaDoc warnings in the nuodb module.

[WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:22: warning: no @param for stringOfDouble [WARNING] public static Double nuoDBConvertStringToDouble(String stringOfDouble) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:22: warning: no @return [WARNING] public static Double nuoDBConvertStringToDouble(String stringOfDouble) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @param for referenceToCast [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @param for length [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @return [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:43: warning: no @param for bigDecimal [WARNING] public static String nuoDBConvertBigDecimalToPlainString(BigDecimal bigDecimal) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:43: warning: no @return [WARNING] public static String nuoDBConvertBigDecimalToPlainString(BigDecimal bigDecimal) { [WARNING] ^

Contributor

venushka commented Oct 20, 2017

Already useful, there are some JavaDoc warnings in the nuodb module.

[WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:22: warning: no @param for stringOfDouble [WARNING] public static Double nuoDBConvertStringToDouble(String stringOfDouble) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:22: warning: no @return [WARNING] public static Double nuoDBConvertStringToDouble(String stringOfDouble) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @param for referenceToCast [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @param for length [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:33: warning: no @return [WARNING] public static Cast nuoDBCastToString(AliasedField referenceToCast, int length) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:43: warning: no @param for bigDecimal [WARNING] public static String nuoDBConvertBigDecimalToPlainString(BigDecimal bigDecimal) { [WARNING] ^ [WARNING] /home/travis/build/alfasoftware/morf/morf-nuodb/src/main/java/org/alfasoftware/morf/jdbc/nuodb/NuoDBHelper.java:43: warning: no @return [WARNING] public static String nuoDBConvertBigDecimalToPlainString(BigDecimal bigDecimal) { [WARNING] ^

@badgerwithagun

This comment has been minimized.

Show comment
Hide comment
@badgerwithagun

badgerwithagun Oct 20, 2017

Member

My thinking is that the wish to suppress tests logging is a very Travis-specific issue which shouldn't affect tests run in an IDE, local builds or Alfa's internal CI where having everything in the build log is usually more convenient than scrabbling around downloading build artifacts.

Could we perhaps try and delete all these log config files in the main code but just drop them in during Travis builds?

Member

badgerwithagun commented Oct 20, 2017

My thinking is that the wish to suppress tests logging is a very Travis-specific issue which shouldn't affect tests run in an IDE, local builds or Alfa's internal CI where having everything in the build log is usually more convenient than scrabbling around downloading build artifacts.

Could we perhaps try and delete all these log config files in the main code but just drop them in during Travis builds?

Using separate log4j configuration file for Maven build writing to lo…
…g files instead of console, and restoring log4j configuration for modules to write to console by default.
@venushka

This comment has been minimized.

Show comment
Hide comment
@venushka

venushka Oct 20, 2017

Contributor

My thinking is that the wish to suppress tests logging is a very Travis-specific issue which shouldn't affect tests run in an IDE, local builds or Alfa's internal CI where having everything in the build log is usually more convenient than scrabbling around downloading build artifacts.

Its a Maven build specific issue in my opinion as the excessive amount of logs printed in the middle of the build log make is very difficult to spot any warnings in the build such as character encoding warning, javadoc warnings, deprecation warnings, etc. We are already doing this for Alfa internal builds when running with jacoco runner, so this is not any different actually.

Could we perhaps try and delete all these log config files in the main code but just drop them in during Travis builds?

Good point! Got me thinking of finding a way to use a different log4j configuration when running the Maven build, and did indeed find a way. I've changed all the log4j configuration files in test resources to log to console (previously some of them were writing to files and some to console) and added system property to the surefire plugin to use a different log4j configuration file when running the build in coverage-per-test profile, which we only use in CI running with coverage. This would make the logs show in the console when running tests in the IDE and local builds. I think this way the CI will let us know when the test fails and optionally collect junit logs artifacts for investigation and also local builds will remain unchanged.

This pull request now include all the changes.

Contributor

venushka commented Oct 20, 2017

My thinking is that the wish to suppress tests logging is a very Travis-specific issue which shouldn't affect tests run in an IDE, local builds or Alfa's internal CI where having everything in the build log is usually more convenient than scrabbling around downloading build artifacts.

Its a Maven build specific issue in my opinion as the excessive amount of logs printed in the middle of the build log make is very difficult to spot any warnings in the build such as character encoding warning, javadoc warnings, deprecation warnings, etc. We are already doing this for Alfa internal builds when running with jacoco runner, so this is not any different actually.

Could we perhaps try and delete all these log config files in the main code but just drop them in during Travis builds?

Good point! Got me thinking of finding a way to use a different log4j configuration when running the Maven build, and did indeed find a way. I've changed all the log4j configuration files in test resources to log to console (previously some of them were writing to files and some to console) and added system property to the surefire plugin to use a different log4j configuration file when running the build in coverage-per-test profile, which we only use in CI running with coverage. This would make the logs show in the console when running tests in the IDE and local builds. I think this way the CI will let us know when the test fails and optionally collect junit logs artifacts for investigation and also local builds will remain unchanged.

This pull request now include all the changes.

@badgerwithagun

This comment has been minimized.

Show comment
Hide comment
@badgerwithagun

badgerwithagun Oct 21, 2017

Member

I'm still not convinced by the idea of shipping Morf with log4j config files included at all - feels like log configuration should be the responsibility of the application (related to #9), not of Morf itself.

This is much better than what we have now though. Tempted to say let's get this in for now and agonise over the rights and wrongs of our logging setup separately.

Member

badgerwithagun commented Oct 21, 2017

I'm still not convinced by the idea of shipping Morf with log4j config files included at all - feels like log configuration should be the responsibility of the application (related to #9), not of Morf itself.

This is much better than what we have now though. Tempted to say let's get this in for now and agonise over the rights and wrongs of our logging setup separately.

@venushka

This comment has been minimized.

Show comment
Hide comment
@venushka

venushka Oct 21, 2017

Contributor

Actually all the log config fields are in test resources directory, so we are really not shipping them. They are only used for testing. Haven’t checked but I think we only depend on log4j library in test scope as well.

Contributor

venushka commented Oct 21, 2017

Actually all the log config fields are in test resources directory, so we are really not shipping them. They are only used for testing. Haven’t checked but I think we only depend on log4j library in test scope as well.

@badgerwithagun

This comment has been minimized.

Show comment
Hide comment
@badgerwithagun

badgerwithagun Oct 24, 2017

Member

It is in test scope, you're right! OK, great, I'm going to approve this.

Member

badgerwithagun commented Oct 24, 2017

It is in test scope, you're right! OK, great, I'm going to approve this.

@badgerwithagun badgerwithagun merged commit 048e493 into alfasoftware:master Oct 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment