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

[BEAM-177] Upload JaCoCo coverage data to Coveralls from Travis-CI #138

Closed
wants to merge 1 commit into from

Conversation

kennknowles
Copy link
Member

No description provided.

@kennknowles
Copy link
Member Author

Example result: https://coveralls.io/jobs/13374509

@kennknowles
Copy link
Member Author

The important bit is browser integration into code review. For coveralls there seems to be this.

@kennknowles kennknowles changed the title Upload JaCoCo coverage data to Coveralls from Travis-CI [BEAM-177] Upload JaCoCo coverage data to Coveralls from Travis-CI Apr 6, 2016
@kennknowles
Copy link
Member Author

The failure is in the RunnableOnService tests, unrelated to the contents of the PR. The job run by the test seems to have a non-unique job name.

@@ -31,6 +31,9 @@ before_install:
install:
- travis_retry mvn -B install clean -U -DskipTests=true

after_success:
Copy link
Member

Choose a reason for hiding this comment

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

after_success logically comes after script. Move below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@davorbonaci
Copy link
Member

R: @davorbonaci

Meta-comment: I'd like Jenkins to do this instead of Travis. Not sure what will happen if they both attempt to do it.

@@ -31,6 +31,9 @@ before_install:
install:
- travis_retry mvn -B install clean -U -DskipTests=true

after_success:
- mvn jacoco:report coveralls:report

Copy link
Member

Choose a reason for hiding this comment

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

Also, not sure what will happen when Travis tries to do this from its test matrix. We should probably make sure it runs just once, not 4 times.

Copy link
Member Author

Choose a reason for hiding this comment

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

It runs just once, for the first job. Of course, coverage values could actually differ, but I think that is beyond coveralls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicitly running it just on Java 8 configuration, since that covers more.

@kennknowles
Copy link
Member Author

Re: Travis vs Jenkins I don't care so much in this case. It should be fast, so it does not slow down code review waiting for results. If Jenkins is faster, then it is definitely the best choice.

But I have a different stance on the meta-issue:

  1. Travis (or equivalent - even our own .jenkins.sh) piggybacks on our existing processes perfectly.
    • Process: Code review of the configuration changes. This implies higher quality results and more transparency about when and why changes occur.
    • Permissions: Anyone can propose a change in a PR, and any committer can accept it.
    • Tools: Text editor, git. And the UI, while less powerful overall, is easier for newcomers.
  2. Travis has good integrations with lots of services. For example, you can use coveralls without a repo token :-)
  3. Travis is trivial to configure.

Jenkins has capabilities that Travis does not have:

  1. It has a nice test result UI. This is the only reason I can think of to run unit tests there.
  2. You can configure a build that won't just obey the instructions in a PR, so it can do more sensitive things or be very expensive to run. (integration tests, submit queue)
  3. Lots of plugins.
  4. (Hopefully) lower latency for uploading coverage.

These are compelling. We should use Jenkins for these things. But I don't see a strong reason to use it for more than that, especially since there are major downsides - no source control of config, more complex config, having to issue tickets to infra for some changes, silent back-channel changes, no review of config changes, no presubmit of config changes, no ability to synchronously alter config and code, ...

For now, I'd be happy to merge PRs and get immediate benefit from Travis until our Jenkins config is offering equivalent capabilities. Perhaps it will be quite soon, but the cost is zero either way.

tgroh pushed a commit to tgroh/beam that referenced this pull request Apr 7, 2016
@kennknowles
Copy link
Member Author

BTW examining the latency, Jenkins is much much lower latency so we should go with Jenkins for this. Travis in the meantime.

@kennknowles
Copy link
Member Author

I should have remembered earlier that we can have both: https://wiki.jenkins-ci.org/display/JENKINS/Travis+YML+Plugin. Or, somewhat fewer of the goals attained via https://github.com/samrocketman/jervis

@kennknowles
Copy link
Member Author

PTAL. Tests pass and we can start getting some benefit for all reviews that happen long enough after a PR shows up.

@davorbonaci
Copy link
Member

Please test.

[ERROR] Failed to execute goal org.eluder.coveralls:coveralls-maven-plugin:4.1.0:report (default-cli) on project parent: I/O operation failed: No coverage report files found -> [Help 1]

@kennknowles
Copy link
Member Author

The problem is this: Running the JaCoCo agent is incompatible with setting forkCount=0

@davorbonaci
Copy link
Member

The same error still seems to occur.

@kennknowles
Copy link
Member Author

Yes, I did not remove the setting of forkCount=0. Why was it added?

@davorbonaci
Copy link
Member

I believe @dhalperi added it -- Travis build was failing without it at some point.

@davorbonaci
Copy link
Member

  • Jacoco doesn't work if forkCount is set to 0. See here for more details.
  • We set forkCount to zero in the Travis environment to control memory usage. Empirically, if we set it to one, it will fail with out-of-memory issue almost always.

Basically, to move forward, we'd have to investigate memory usage of individual tests and get it down. Then, we can increase `forkCount, and finally enable Jacoco in Travis.

Given that it "somewhat works" in Jenkins right now, that Jenkins is generally preferable over Travis, that it seems easier to fully fix Jenkins than Travis, etc., I'd just probably close this pull request.

@kennknowles
Copy link
Member Author

SGTM. Probably a red flag that we blow memory running unit tests, though.

@dhalperi
Copy link
Contributor

It's actually simpler than that. Since Maven does not copy memory limits from the root process to the forked processes, Java never garbage collects (since it does not think it needs to). The VM happily grows the (non-leaking) memory until Travis kills it well before the Java memory limit is reached.

@kennknowles
Copy link
Member Author

I see. This can be fixed via maven config in a few ways I think. I suppose it may not be worth making the pom more complex.

@kennknowles kennknowles deleted the coveralls branch November 10, 2016 03:09
iemejia referenced this pull request in iemejia/beam Jan 12, 2018
mareksimunek pushed a commit to mareksimunek/beam that referenced this pull request May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants