Skip to content
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

Travis: Build new binding pull requests with tests #5389

Merged
merged 4 commits into from
Jun 28, 2019

Conversation

davidgraeff
Copy link
Member

@davidgraeff davidgraeff commented Apr 4, 2019

travisbuddy

This PR adds travisbuddy to .travis.yml. That service add (update) a comment to a PR on a build failure with the log attached. This is an intermediate step until we have full github checks api support and a curl command figured out to report SAT findings and Test results to the "Checks" tab. See also #4935

Partial build

The PR commits are analysed and if exactly one new directory is added to bundles/ then a build with tests and checks is performed for that new bundle otherwise the full repo build without tests and checks is executed as before.

WIP

  • This PR includes an absolute irrelevant commit for testing the bundles/ changed detection
  • Karaf feature test is not yet performed in the partial build
  • SAT should fail if a new binding is not added to the bom or the parent pom.xml or the karaf feature file
  • Maven is still showing downloads although I have -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn set. That clutters the build log which would otherwise only show SAT and test reports

Advantage for a new binding pull request

  • Build time of under 5 minutes
  • Clean log with only relevant SAT and test findings for that new PR. We can even increase the SAT report level and make more warnings fail the build.
  • Review time can be spend on more important questions like architecture and technology patterns

Signed-off-by: David Graeff david.graeff@web.de

@davidgraeff davidgraeff requested a review from a team as a code owner April 4, 2019 11:29
@TravisBuddy
Copy link

Travis tests have failed

Hey @davidgraeff,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

buildci.sh
buildci.sh: command not found
TravisBuddy Request Identifier: e0396110-5714-11e9-979a-6d93f48c24a0

@davidgraeff davidgraeff force-pushed the travisex branch 2 times, most recently from 240e34a to 203a57d Compare April 4, 2019 20:34
@TravisBuddy
Copy link

Hey @davidgraeff,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: a6775c70-5719-11e9-979a-6d93f48c24a0

@martinvw
Copy link
Member

martinvw commented Apr 5, 2019

.. out to report SAT findings ...

That used to work briefly on Jenkins.

@kaikreuzer did you set this up?

@kaikreuzer
Copy link
Member

@martinvw Yes, Jenkins is configured to publish the SAT reports of the PR builds - but it looks in target/*, where the report does not exist any longer. Where is it now?

@wborn wborn added the work in progress A PR that is not yet ready to be merged label Apr 6, 2019
@davidgraeff davidgraeff added the infrastructure Build system and Karaf related issues and PRs label May 6, 2019
@martinvw
Copy link
Member

@davidgraeff is this something which can be wrapped up, it looks promising.

@davidgraeff
Copy link
Member Author

davidgraeff commented Jun 28, 2019

I have outlined what is still missing in the WIP section. I would like to have karaf feature tests, but I do not know the maven goal and target to do that. And the karaf maven plugin page is absolutely not helpful for a non-maven guy :/

A second issue is that maven still outputs download progress. I really don't know what is going on in maven developers mind and judging by the code that I have inspected I'd say not much at all. They could at least have used different log classes for different types of output.
Maybe they expect people to use piped log filters. I honestly don't know.

I wish to have a clean build log for travisbuddy, so that even newbies can easily identify and tell apart notes, warnings and build step logs.

@martinvw
Copy link
Member

martinvw commented Jun 28, 2019 via email

@davidgraeff
Copy link
Member Author

What you have found looks promising, that's what I had in mind

@martinvw
Copy link
Member

Karaf feature test is not yet performed in the partial build
SAT should fail if a new binding is not added to the bom or the parent pom.xml or the karaf feature file

I suppose to separate that and merge what you already included, shall I also remove the new fake binding?

@martinvw
Copy link
Member

@davidgraeff shall I also rebase it? You might then be better of removing your local copy I IMHO

@martinvw martinvw removed the work in progress A PR that is not yet ready to be merged label Jun 28, 2019
@martinvw martinvw changed the title [WIP] Travis: Build new binding pull requests with tests Travis: Build new binding pull requests with tests Jun 28, 2019
David Graeff added 3 commits June 28, 2019 13:54
Signed-off-by: David Graeff <david.graeff@web.de>
…uration part. Use daily snapshot updates

Signed-off-by: David Graeff <david.graeff@web.de>
Signed-off-by: David Graeff <david.graeff@web.de>
…ngs from the reporting utility

Signed-of-by: Martin van Wingerden <martin@martinvw.nl>
@martinvw
Copy link
Member

martinvw commented Jun 28, 2019

@openhab/2-x-add-ons-maintainers

The two running builds should help us decide whether its time to merge:

Full build:

https://travis-ci.org/openhab/openhab2-addons/jobs/551749200

Single-binding build: (Ran for 5 min 13 sec)

https://travis-ci.org/openhab/openhab2-addons/jobs/551752246

@TravisBuddy
Copy link

Hey @davidgraeff,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: e8583c50-999e-11e9-8610-39232d9ecaa4

@martinvw
Copy link
Member

martinvw commented Jun 28, 2019

I think that is such a big improvement that we should just do it. We could create issues for the open points and we SHOULD NOT FORGET to remove the fake binding :-D

Could we also use this moment to re-enable the tests (not the itTests)

Copy link
Member

@martinvw martinvw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy but we shouild not forget to remove the fake binding

@TravisBuddy
Copy link

Hey @davidgraeff,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 3eee7f40-99a2-11e9-8610-39232d9ecaa4

@davidgraeff
Copy link
Member Author

Perfect. I agree to merge asap.

@J-N-K
Copy link
Member

J-N-K commented Jun 28, 2019

Not directly related but similar topic: we should get SAT working again, this especially important for new contributions.

@martinvw
Copy link
Member

Not directly related but similar topic: we should get SAT working again, this especially important for new contributions.

AFAIK it just works, I was at least able to run it for the single binding. I didn’t test the full build though.

@J-N-K
Copy link
Member

J-N-K commented Jun 28, 2019

SAT is not including all reports and not working for bindings with internal libraries (PR Open)

@davidgraeff
Copy link
Member Author

davidgraeff commented Jun 28, 2019

AFAIK it just works, I was at least able to run it for the single binding. I didn’t test the full build though.

The full build without SAT (and tests?) apparently already takes 40 minutes. Because maven doesn't perform SAT checks in parallel to build steps, that will probably increase to an unhandable long time.

So I think it never make sense to run SAT for a full build in this repo. (Not even the distro would need that. I mean what will the distro guys do if a binding in here fails on SAT checks.)

With this PR merged, every binding that will be touched from then on will run SAT (except the ones with internal libraries as J-N-K has pointed out).

@martinvw
Copy link
Member

@wborn / @Hilbrand / @cweitkamp / @J-N-K I think that if you are okay, then its ready to be merged, I stripped of the test-commit.

@J-N-K
Copy link
Member

J-N-K commented Jun 28, 2019

Manually verified Signed-Off.

@TravisBuddy
Copy link

Hey @davidgraeff,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: ab623700-99bd-11e9-8610-39232d9ecaa4

@J-N-K J-N-K merged commit f9fa7f6 into openhab:master Jun 28, 2019
@davidgraeff davidgraeff deleted the travisex branch June 28, 2019 18:51
@martinvw
Copy link
Member

martinvw commented Jun 28, 2019

@davidgraeff there is a little flaw, if the author does not keep the master branch of his own repo up-to-date it still triggers a full rebuild. We should try to look into that.

On my feature/4633-improve-thermostat3-support branch, git diff --diff-filter=A --dirstat=files,0 master...HEAD returns:

   0.0% .github/ISSUE_TEMPLATE/
   0.0% .github/
   0.0% bom/openhab-addons/
   0.0% bom/openhab-core-index/
   0.0% bom/runtime-index/
   0.0% bom/test-index/
   0.0% bom/
   0.0% bundles/org.openhab.binding.airquality/src/main/feature/
   0.0% bundles/org.openhab.binding.airquality/src/main/java/org/openhab/binding/airquality/internal/discovery/
   0.0% bundles/org.openhab.binding.airquality/src/main/java/org/openhab/binding/airquality/internal/handler/
   0.0% bundles/org.openhab.binding.airquality/src/main/java/org/openhab/binding/airquality/internal/json/
   0.0% bundles/org.openhab.binding.airquality/src/main/java/org/openhab/binding/airquality/internal/
   0.0% bundles/org.openhab.binding.airquality/src/main/resources/ESH-INF/binding/
   0.0% bundles/org.openhab.binding.airquality/src/main/resources/ESH-INF/thing/
   0.0% bundles/org.openhab.binding.airquality/
   0.0% bundles/org.openhab.binding.airvisualnode/src/main/feature/
   0.0% bundles/org.openhab.binding.airvisualnode/src/main/java/org/openhab/binding/airvisualnode/internal/config/
   0.0% bundles/org.openhab.binding.airvisualnode/src/main/java/org/openhab/binding/airvisualnode/internal/discovery/
......
   0.0% itests/org.openhab.io.hueemulation.tests/
   0.0% itests/org.openhab.persistence.mapdb.tests/src/main/java/org/openhab/persistence/mapdb/
   0.0% itests/org.openhab.persistence.mapdb.tests/
   0.0% itests/
   0.0% licenses/epl-2.0/
   0.0% src/etc/
   0.0% tools/static-code-analysis/checkstyle/
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your diff.renameLimit variable to at least 7601 and retry the command.

}

# Determine if this is a new addon -> Perform tests + integration tests and all SAT checks with increased warning level
CHANGED_DIR=`git diff --diff-filter=A --dirstat=files,0 master...HEAD bundles/ | sed 's/^[ 0-9.]\+% bundles\///g' | grep -o -P "^([^/]*)" | uniq`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have a special build for people changing only files in a single module, now it only focuses on either new contributions or it does the complete build.

@martinvw
Copy link
Member

@davidgraeff there is a little flaw, if the author does not keep the master branch of his own repo up-to-date it still triggers a full rebuild. We should try to look into that.

I found a possible solution, there is the option to get the commit range which is part of the PR as an input: $TRAVIS_COMMIT_RANGE I'm adapting the scripts and will also add support for only changes in a single module, I think that I would just handle it as one, unless someone disagrees?

@martinvw
Copy link
Member

I included the changes mentioned above in #5765

knikhilwiz pushed a commit to knikhilwiz/openhab2-addons that referenced this pull request Jul 3, 2019
* Travis: Build new binding pull requests with tests
* Move shell script into own file and keep the yaml file for the configuration part. Use daily snapshot updates
* Overwrite travis default behaviour

Signed-off-by: David Graeff <david.graeff@web.de>

* Some finetuning to filter out download progress and include all warnings from the reporting utility

Signed-of-by: Martin van Wingerden <martin@martinvw.nl>
@wborn wborn added this to the 2.5 milestone Jul 30, 2019
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
* Travis: Build new binding pull requests with tests
* Move shell script into own file and keep the yaml file for the configuration part. Use daily snapshot updates
* Overwrite travis default behaviour

Signed-off-by: David Graeff <david.graeff@web.de>

* Some finetuning to filter out download progress and include all warnings from the reporting utility

Signed-of-by: Martin van Wingerden <martin@martinvw.nl>
Signed-off-by: Maximilian Hess <mail@ne0h.de>
@wborn wborn added the test label Oct 1, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019
* Travis: Build new binding pull requests with tests
* Move shell script into own file and keep the yaml file for the configuration part. Use daily snapshot updates
* Overwrite travis default behaviour

Signed-off-by: David Graeff <david.graeff@web.de>

* Some finetuning to filter out download progress and include all warnings from the reporting utility

Signed-of-by: Martin van Wingerden <martin@martinvw.nl>
@wborn wborn removed this from the 2.5 milestone Dec 8, 2019
@wborn wborn added this to the 2.5 milestone Dec 16, 2019
tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020
* Travis: Build new binding pull requests with tests
* Move shell script into own file and keep the yaml file for the configuration part. Use daily snapshot updates
* Overwrite travis default behaviour

Signed-off-by: David Graeff <david.graeff@web.de>

* Some finetuning to filter out download progress and include all warnings from the reporting utility

Signed-of-by: Martin van Wingerden <martin@martinvw.nl>
Signed-off-by: Tim Roberts <timmarkroberts@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Build system and Karaf related issues and PRs test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants