zeromq / jeromq

JeroMQ is a pure Java implementation of the ZeroMQ messaging library, offering high-performance asynchronous messaging for distributed or concurrent applications.
https://zeromq.org
Mozilla Public License 2.0
2.36k stars 483 forks source link

Enforce code resilience and usage #472

Closed fredoboulo closed 5 years ago

fredoboulo commented 7 years ago

These last weeks, bugs impacting major features were fixed.

The big thing for me is that these ultra important major features were absolutely never tested: downgrading capabilities, socket reconnection, Android compatibility. I take the responsibilities of my mistakes, and I would like to go forward on that matter and be constructive about it.

I propose to enforce testing scenarii so no commit will provoke a regression of that level.

trevorbernard commented 7 years ago

I completely agree and endorse this effort. We're certainly blind in the areas you mentioned.

daveyarwood commented 7 years ago

I also think this is a great idea, very worthwhile.

Re: Travis CI for Android, it looks like that ought to be fairly easy to set it up so that we run the build and tests on a variety of Android SDK versions: https://docs.travis-ci.com/user/languages/android/

It seems like it might be a good idea to set up automated end-to-end testing as a separate repo in the zeromq org, since that would be testing not only JeroMQ but other flavors of libzmq. If some of those tests fail, it might not necessarily be an indicative of a problem with JeroMQ, so we wouldn't want those failures to block the development of JeroMQ.

fredoboulo commented 7 years ago

That sounds promising!

For downgrading capabilities, I was thinking about a set of well-known and stable releases of libzmq, that implement flawlessly the different protocols. So if there is a miscommunication, it can be certified that it comes from the tested side.

daveyarwood commented 7 years ago

Ah, that makes sense. Would it be fair to consider libzmq (using the C library directly) the "gold standard" against we measure other ZMQ implementations?

fredoboulo commented 7 years ago

It's fair enough for me. We can always go in full blossomed matrix mode at a latter step, but I would be more than pleased having comms tested with libzmq in all protocol versions and formats (v0 to v3).

fredoboulo commented 7 years ago

Just proposed PR 479 to add coverage to the project. So... coverage status is ... 74% for the whole project.

org.zeromq is mostly dropping this figure because a lot of trivial methods are not tested. zmq least covered classes are Options and SocketBase, the latter mostly because tipc code is not used..

sigiesec commented 7 years ago

@fredoboulo That's really great :) I am not familiar on how to do this for Java, but would it be possible to publish the coverage results from travis-ci to coveralls.io ?

fredoboulo commented 7 years ago

Someone with better infrastructure knowledge on travis would provide a better answer, but when I previously tried to introduce jacoco coverage by default, it was crashing the testing machines... It works fine on my computer though, with the command : mvn clean package -Pcoverage.

sigiesec commented 7 years ago

Oh, I somehow assumed that the coverage measurements are already done on travis-ci by your changes. Then that needs to be done before anything can be published, of course ;) @jcoeltjen Do you know how this could be done on travis-ci?

daveyarwood commented 7 years ago

I did some quick googling, and it looks like the thing to do for Java projects is to install this Coverall Maven plugin: https://github.com/trautonen/coveralls-maven-plugin

The README says it has built-in support for Travis CI with no configuration needed out of the box. Sounds kind of promising to me.

I think someone from the org will have to sign up for a free Coveralls account too: https://coveralls.io/sign-up

sigiesec commented 7 years ago

The sign-up link is for a paid account. I enabled zeromq/jeromq on coveralls, but I do not have access to for zeromq/jeromq on travis-ci, so I cannot enter the repository token from coveralls there.

daveyarwood commented 7 years ago

I think that's just a matter of tweaking .travis.yml, no?

sigiesec commented 7 years ago

Technically that were possible, but the repository token should not be accessible publicly.

daveyarwood commented 7 years ago

It is possible to encrypt environment variables so only Travis has access to them: https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml

How does the repository token work, can it be set in an environment variable?

fredoboulo commented 7 years ago

@sigiesec do you have any pointers about how libzmq is testing protocol downgrades and reconnection behavior?

sigiesec commented 7 years ago

Protocol downgrades are not tested at all at the moment :disappointed:

I think reconnection behaviour is also not tested explicitly.

I don't know if someone performs test on a regular basis that are not part of the repository.

sigiesec commented 7 years ago

Yes, the token can be stored in an encrypted environment variable. the name is COVERALLS_REPO_TOKEN. Here is a description of encrypted environment variables: https://docs.travis-ci.com/user/environment-variables/#Defining-encrypted-variables-in-.travis.yml but someone with access to both the coveralls and the travis project must encrypt the token.

daveyarwood commented 7 years ago

From what I can tell, COVERALLS_REPO_TOKEN used to be required, but now Coveralls + Travis CI works out of the box for public repos. It looks like this might be just a matter of adding the Coveralls Maven plugin to our pom.xml and the Coveralls submit task to our .travis.yml.

I'll throw together a PR and maybe we can see if this works through trial and error. I'd say that I about 75% know what I'm doing 😄

sigiesec commented 7 years ago

Ah, great, that would simplify things :)

fredoboulo commented 6 years ago

I added tests for downgrading lately, so we should be enforcing that part from now on...

daveyarwood commented 5 years ago

I feel the urge to break this issue up into smaller, discrete issues, as it is pretty broad. We have already made some progress on this, which is fantastic.

@fredoboulo What would you say the action items are at this point? Did I miss anything below?

A couple that I can think of are:

fredoboulo commented 5 years ago

We are still not covering automagic reconnection of sockets in tests.

Apart from that, sounds good to me.

We are currently unit testing downgrading capabilities. But a cross-project end-to-end system to help us assessing the compatibility between breeds of Zeromq would be fantastic as well.

daveyarwood commented 5 years ago

Broke this issue up into the 3 new issues above.