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

Update TRAVIS in order to test Java tools #188

Merged
merged 1 commit into from Nov 29, 2016
Merged

Update TRAVIS in order to test Java tools #188

merged 1 commit into from Nov 29, 2016

Conversation

mcoolive
Copy link
Contributor

@mcoolive mcoolive commented Nov 23, 2016

  • Specify the version of Python
  • Specify the version of the JDK, and install it
  • Add execution of existing Java tests. Run once to save time (on the
    slave that checks pyjama).

This change is Reviewable

- TOXENV=pylama
- TOXENV=pypy-coverage
- TOXENV=py27-coverage
- JAVA=true TOXENV=pylama # We build Java once only
Copy link
Contributor

Choose a reason for hiding this comment

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

Running the java tests in the pylama env is kind of weird.
What do you think about having a fourth case with JAVA=true and TOXENV being null ?
Also maybe we should make JAVA=false implicit.

@mcoolive
Copy link
Contributor Author

.travis.yml, line 33 at r1 (raw file):

Previously, iksaif (Corentin Chary) wrote…

Running the java tests in the pylama env is kind of weird.
What do you think about having a fourth case with JAVA=true and TOXENV being null ?
Also maybe we should make JAVA=false implicit.

Currently if JAVA is not specified in the travis matrix, JAVA is tested (so it acts as if true was the default value). I can change that easily.


Comments from Reviewable

@mcoolive
Copy link
Contributor Author

.travis.yml, line 33 at r1 (raw file):

Previously, mcoolive (Cyril Martin) wrote…

Currently if JAVA is not specified in the travis matrix, JAVA is tested (so it acts as if true was the default value). I can change that easily.

About the mangle Java/Pylama.
I agree mangle Java and Pylama doesn't make sense. First I look for a trick to execute JAVA tests once. I put them with pyjama (and not with other target) because it is the shortest build on travis, so it doesn't affect the build time too much (in fact, the build time increases by 30 sec because we install JDK8 on each env...)

Separate them seams cleaner. But I don't know how to achieve that in a clean way. What do you mean by "TOXENV being null"? As far I understand, when TOXENV is undefined by travis, tox plays all envs declared in tox.ini. I could wrap the tox command in a "if $PYTHON" or "if ! $ JAVA", but it seams dirty and complex to me.


Comments from Reviewable

@iksaif
Copy link
Contributor

iksaif commented Nov 28, 2016

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


.travis.yml, line 33 at r1 (raw file):

Previously, mcoolive (Cyril Martin) wrote…

About the mangle Java/Pylama.
I agree mangle Java and Pylama doesn't make sense. First I look for a trick to execute JAVA tests once. I put them with pyjama (and not with other target) because it is the shortest build on travis, so it doesn't affect the build time too much (in fact, the build time increases by 30 sec because we install JDK8 on each env...)

Separate them seams cleaner. But I don't know how to achieve that in a clean way. What do you mean by "TOXENV being null"? As far I understand, when TOXENV is undefined by travis, tox plays all envs declared in tox.ini. I could wrap the tox command in a "if $PYTHON" or "if ! $ JAVA", but it seams dirty and complex to me.

What I mean is that this would look cleaner:

  • TOXENV=pylama
  • TOXENV=pypy-coverage
  • TOXENV=py27-coverage
  • JAVA=true

Comments from Reviewable

@mcoolive
Copy link
Contributor Author

.travis.yml, line 33 at r1 (raw file):

Previously, iksaif (Corentin Chary) wrote…

What I mean is that this would look cleaner:

  • TOXENV=pylama
  • TOXENV=pypy-coverage
  • TOXENV=py27-coverage
  • JAVA=true

Done.


Comments from Reviewable

- Specify the version of Python
- Specify the version of the JDK, and install it
- Add execution of existing Java tests. Run once to save time (on the
slave that checks pyjama).
@iksaif
Copy link
Contributor

iksaif commented Nov 29, 2016

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iksaif iksaif merged commit 9e3a21d into criteo:master Nov 29, 2016
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

2 participants