-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add support for maven.test.redirectTestOutputToFile #78
Add support for maven.test.redirectTestOutputToFile #78
Conversation
Hi @metteo, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you. |
@cla-bot[bot] check |
The cla-bot has been summoned, and re-checked this pull request! |
@katrinsharp FYI |
@cheeseng do you process PRs or should I ask someone else? |
@metteo Er, I don't work on this repo much, perhaps @katrinsharp ? |
@cheeseng thanks for the response. I already called @katrinsharp but she didn't respond. Maybe @bvenners could help? |
e92d4cf
to
1de74e3
Compare
Sorry, was sidetracked by some stuff. I'll review, merge and build this week. |
No problem. I expected that you will need some time to process the PR. Just got a little worried that I didn't get any response mentioning it will be processed in foreseeable future 😁 |
Hi @metteo Thank you for submitting your PR. Does your PR need any updates to main README? Any build instructions that would be useful? |
Release related plugins are in separate |
Using JDK newer than 8 might be tricky together with current maven compiler setup (source=8, target=8 instead of release=8). That's why github action I added is using JDK 8 for CI. |
Should I update the README with above info? |
@katrinsharp any updates? |
My apologies, yes. Please resolve conflicts as I've merged another PR, and I'll publish a new version this weekend. Thank you! |
1de74e3
to
cef20bb
Compare
@katrinsharp PR rebased. |
- disable ANSI codes if enabled - allow customization of the output file name - fix IT verify scripts to catch empty line case - upgrade java version to 8 - IT which verifies output is redirected
cef20bb
to
5068778
Compare
@katrinsharp I saw you merged another PR so I rebased this one to resolve conflicts. I would expect this gets merged next. If you are hesitant in merging, please let me know why so I can apply appropriate changes. |
@metteo I'd like to go over your PR once again to make sure it is good-to-go and then will merge. Please stay tuned. Thank you. |
@bvenners I went through the PR changes and tested run the integration test with:
and when I go into target/it/redirect-output/target/scalatest-reports I can see the scalatest-output.txt which contains the following:
Seems working great, @bvenners please review and consider approving so I can go ahead and merge this. Thanks! |
@cheeseng please try |
Hi
This change will make the build logs shorter for cases when the tests produce lots of logs.
Please review.
Details:
Regards
Greg