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

[ci] [GSoC] Integrating the danger and pmdtester to travis CI #1254

Merged
merged 1 commit into from Jul 30, 2018

Conversation

Projects
None yet
4 participants
@djydewang
Member

djydewang commented Jul 23, 2018

  • We only perform the regression testing when mvn verify runs successfully and the PR is mergeable.
  • When contributor have submitted the new commits, old and new regression diff reports are both available.
(
set +e
log_info "Running danger"
bundle exec danger --verbose

This comment has been minimized.

@adangel

adangel Jul 23, 2018

Member

This needs to be without "bundle exec" now.
But I can't tell you, why it didn't work, when you used "bundle install" earlier - all the gems have been installed according to the logs...

This comment has been minimized.

@djydewang

djydewang Jul 24, 2018

Member

@adangel This seems to be a problem with pmdtester, because there is no such file as pmdtester.rb in the lib directory of pmdtester, I now temporarily solve this issue with require 'pmdtester/runner'

@djydewang djydewang force-pushed the djydewang:danger branch 3 times, most recently from 89bca9e to 1cf49f9 Jul 24, 2018

@pmd-bot

This comment has been minimized.

Contributor

pmd-bot commented Jul 24, 2018

1 Message
📖 Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
Please check the regression diff report to make sure that everything is expected
No java rules are changed!

Generated by 🚫 Danger

@djydewang djydewang force-pushed the djydewang:danger branch 3 times, most recently from 3f82596 to 9694bd8 Jul 24, 2018

@djydewang

This comment has been minimized.

Member

djydewang commented Jul 24, 2018

@djydewang djydewang force-pushed the djydewang:danger branch 3 times, most recently from 5ccb3a8 to cdf7c20 Jul 24, 2018

@djydewang djydewang changed the title from [GSoC] Integrating the danger and pmdtester to travis CI to [ci][GSoC] Integrating the danger and pmdtester to travis CI Jul 24, 2018

@djydewang djydewang force-pushed the djydewang:danger branch 2 times, most recently from 6ac7ab0 to f102e6d Jul 24, 2018

@adangel

This is almost mergeable, just a few changes (most notably to enable the actual build again for PRs).

Could you additionally rebase/squash the commits into one single commit? I think, we don't need the whole history and trials on developing this. I now, it was not easy to develop this, commit, push and wait for the result on travis... I'm glad you managed it. Good job!

@@ -49,7 +49,12 @@ MVN_BUILD_FLAGS="-B -V"
if travis_isPullRequest; then

log_info "This is a pull-request build"
./mvnw verify $MVN_BUILD_FLAGS
# ./mvnw verify $MVN_BUILD_FLAGS

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

We'll need to keep this line in and not commented 😄

# ./mvnw verify $MVN_BUILD_FLAGS
(
set +e
log_info "Running danger"

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

There is some wrong indentation, probably tabs. Just use spaces here.

argv = ['-r', './pmd', '-b', "#{ENV['TRAVIS_BRANCH']}", '-p', 'FETCH_HEAD', '-m', 'online']
Process.fork do
begin
runner = PmdTester::Runner.new(argv)

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

I actually don't like this interface very much, how we call the PmdTester - since it doesn't give us any benefit from just calling the command via backticks (`pmdtester -r ./pmd ....`).
But that's something for later, improving the API of pmdtester - it works for now.
Is this the same, what you mean with your comment "# Optimize: get more information from runner, e.g. introduce new pmd errors?"?

This comment has been minimized.

@djydewang

djydewang Jul 30, 2018

Member

Yes, this is for future expansion. e.g adding a new danger comment about whether the PR intruduce a new PMD error.

Dangerfile Outdated
return
end

`tar -czf #{tar_filename} diff/`

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

You are using here the option "z", so the tar is also gzipped.... the filename should then use the extension .tar.gz to avoid confusion...

if $?.success?
@logger.info "Successfully uploaded #{tar_filename} to chunk.io"

# set value of sticky to true and the message is kept after new commits are submited to the PR

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

That's actually a interesting feature 👍

Dangerfile Outdated
message("Please check the [regression diff report](#{report_url.chomp}/diff/index.html) to make sure that everything is expected", sticky: true)
else
@logger.error "Error while uploading #{tar_filename} to chunk.io: #{report_url}"
warn("Uploading the diff report failed, this message is mainly used to remind the mantainers of the PMD.")

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

Small typo: mantainers -> maintainers
And I think, the article "the" before PMD is not needed.

@@ -0,0 +1,6 @@
source 'https://rubygems.org/'

gem 'pmdtester', '~> 1.0.0.pre.beta2'

This comment has been minimized.

@adangel

adangel Jul 26, 2018

Member

Cool, that means, that we don't need to update PMD if we have released a new version of pmdtester (at least, once we have the first version released, we can specify here ~> 1.0 or even ~> 1.

@adangel adangel added this to the 6.7.0 milestone Jul 28, 2018

@djydewang djydewang force-pushed the djydewang:danger branch from a5d0af2 to 2f1d04c Jul 29, 2018

@djydewang djydewang force-pushed the djydewang:danger branch from 2f1d04c to 2b0e9e0 Jul 29, 2018

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jul 30, 2018

This is looking really cool! Amazing work @djydewang!

@adangel adangel changed the title from [ci][GSoC] Integrating the danger and pmdtester to travis CI to [ci] [GSoC] Integrating the danger and pmdtester to travis CI Jul 30, 2018

@adangel adangel merged commit 2b0e9e0 into pmd:master Jul 30, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
danger/danger All green. Woo!
Details

adangel added a commit that referenced this pull request Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment