test: update and document java requirements for gradle #5435

jGleitz · 2020-02-11T09:35:42Z

The output of java -version for Windows is taken from my Linux machine, I hope that’s fine.

JamieMagee · 2020-02-11T09:43:21Z

docs/development/local-development.md


 - git
 - nodejs `^10.13.0 || ^12.0.0`
 - yarn `^1.17.0`
 - c++ compiler
 - python `^2.7` with `mock` library
+- java between `8` and `11` (or set env `SKIP_JAVA_TESTS` to `true`)


Your previous PR showed that Java 13 was fine for Gradle 6. Maybe the easiest thing is to just specify Java 8+?

I'm running Java 13 on OSX and just found a failed test due to this. Is it not possible to run the Gradle 5 test with Java 13?

I see:

const gradleJavaVersionSupport = { 5: { min: 8, max: 12 }, 6: { min: 8, max: 13 }, };

I know this is a documentation PR, but could it be possible to automatically skip those tests if the Java version isn't compatible? I don't want to cause friction to first-time contributors in particular. If we want to make sure we don't end up with a regression in CI, then we could essentially flip the logic so that on CI we set a variable ENFORCE_JAVA_TESTS that makes sure we never skip them

I think I prefer number 1, although I'd prefer if we could make sure they're always being run on CI. Earlier you noted a "synchronous" load, but could it be done with await/async instead so that it's not blocking?

but could it be done with await/async instead so that it's not blocking

Nope, describe blocks are always synchronous: jestjs/jest#2235

Could be combine them within an it, or use beforeAll, etc? And if we don't, can we quantify what the impact is?

I have now changed the behaviour so that test cases are skipped if the Java version does not match or if there is no Java at all.

~~For CI, I have introduced the FAIL_IF_NO_JAVA env to make it more difficult to skip the tests in CI.~~

The tests will fail if the CI environment variable is set and we don’t have an appropriate Java version. However, the tests can still be skipped by setting SKIP_JAVA_TESTS.

Could be combine them within an it, or use beforeAll, etc

I’m not sure what you have in mind. Note that there does not seem to be any way to skip the test once we have entered the it lambda.

can we quantify what the impact is

Surprisingly, I don’t notice any delay when running the test on my machine with this branch (which now tests for Java synchronously). So it’s probably fine.

docs/development/local-development.md

jGleitz · 2020-02-11T22:46:09Z

@viceice You have disabled the Java tests for Windows & Mac in #5448. As a consequence, we would not see if the Java tests break some developer’s workflow. For example, in #5431, I used ./gradlew, which does not work on Windows. Currently, the build fails on Mac if we don’t install Java and don’t skip the Java tests explicitly (-> Java 8 is used, but the error relates to a Java 13 class file. I have no idea what’s up with that.).

The Gradle tests add ~45s execution time (10s to install Java, 35s to execute them). Maybe we could lower that even further if we started to cache ~/.gradle because I think the most time is spent on downloading gradle.

I am fine with whatever you decide, I just want to bring it up. From my point of view, 45s CI time (~12% of current time) are worth to detect anything that could break other people’s workflow.

jGleitz · 2020-02-11T23:08:21Z

CircleCI fails with a seemingly unrelated error:

Summary of all failing tests
 FAIL  lib/manager/git-submodules/extract.spec.ts
  ● lib/manager/gitsubmodules/extract › extractPackageFile() › extract name path mismatch

    TypeError: Cannot read property 'trim' of null

      24 |       `submodule.${submoduleName}.url`,
      25 |     ])
    > 26 |   ).trim();
         |     ^
      27 |   if (!path.startsWith('../')) {
      28 |     return path;
      29 |   }

      at getUrl (lib/manager/git-submodules/extract.ts:26:5)

does anybody have an idea where this error stems from and what to do with it?

rarkins · 2020-02-12T06:03:47Z

does anybody have an idea where this error stems from and what to do with it?

I used my admin privileges to retry and tests on Circle and it seems to have cleared it.

viceice · 2020-02-12T06:37:34Z

@jGleitz I noticed a big slowdown on ci tests an I thought Java was the cause. I'm ok to enable Java tests. But we should only run them on one node version.

I have to fix the workflows anyways, so I will enable Java later today.

We should document, that we only use latest lts version, currently 11. So we do not support Java 8 official. But it may work

viceice · 2020-02-12T06:38:59Z

@rarkins maybe again a cache related fail? Or a missing await which causes trouble too.

viceice · 2020-02-12T06:40:26Z

https://eslint.org/docs/rules/require-await

rarkins · 2020-02-12T07:53:15Z

I think the reason we apply tests cross-platform is to test in case we have any platform-specific breakages. If we think that the chances of the Gradle tests passing on Linux but failing on Mac or Windows is low then let's skip them and save the build minutes.

jGleitz · 2020-02-12T18:37:40Z

If we think that the chances of the Gradle tests passing on Linux but failing on Mac or Windows is low

That’s exactly my point: I do not think that the chances are sufficiently low. I already made one mistake in the test code and since we’re interacting with the OS for spawning processes and are handling files, I also see the potential for mistakes in the production code.

I think @viceice’s idea is good: let’s enable the Java tests on every operating system, but only on one run.

viceice · 2020-02-12T21:16:38Z

I've got a new error while testing locally. Why we explicitly need java 8 < x < 12 ?

I had java 13 on one of my computers, so i got following error:

 This test needs a Java version between 8 and 12

      57 |           javaVersion > supportedJavaVersions.max
      58 |         ) {
    > 59 |           throw Error(
         |                 ^
      60 |             `This test needs a Java version between ${supportedJavaVersions.min} and ${supportedJavaVersions.max}`
      61 |           );
      62 |         }

      at lib/manager/gradle/gradle-updates-report.spec.ts:59:17

I think for local devel we should only assume a minimal java version to fail. When we found a greater version we should only issue a warning for unsupported.

jGleitz · 2020-02-12T21:53:08Z

I've got a new error while testing locally.

Did you have the CI env set? Because if CI is not set, the tests should be skipped instead of failed (it works for me).

Why we explicitly need java 8 < x < 12 ?

Because the Gradle 5 test will fail with Java 13 (Gradle 5 does not support Java 13). I think it’s better to throw an Error explaining what’s going on than to have the developer figure out themselves while the test failed.

jGleitz · 2020-02-12T21:54:53Z

Ah, I just noticed that your error message does not mention the CI environment variable. This most likely means that you did not have the most recent version of this branch checked out.

Note that I force-pushed the branch.

viceice · 2020-02-12T21:58:44Z

OK, np. i got it on another branch. Now downgraded to java 11 lts, as i like to stay on stable at leat some times 🙃

jGleitz · 2020-02-13T13:13:24Z

As far as I can see, this PR is only waiting for @viceice to enable the Java tests on all operationg systems again. Should I maybe do this myself in this PR, so that we can get it merged?

viceice · 2020-02-13T13:27:22Z

@jGleitz go for it

jGleitz · 2020-02-13T14:56:56Z

Done.

.github/workflows/test.yml

docs/development/local-development.md

viceice · 2020-02-13T15:08:57Z

docs/development/local-development.md

  - `curl https://bootstrap.pypa.io/get-pip.py -o get-pip.py`
  - `python get-pip.py`
  - `rm get-pip.py`
  - `python -m pip install mock`
+- Install [Java](https://adoptopenjdk.net/?variant=openjdk11)


Maybe we should only specify to install latest java tls? eg adoptjdk, openjdk or any other compatible.

I am not sure what you are suggesting. The text only says “Install Java”. I linked to one specific distribution of Java for those people that are not familiar with the discussion of different Java distributions.

If your comment is about the ?variant=openjdk11 part, I added this because the AdoptOpenJDK page pre-selects Java 8 otherwise. With the link I gave, you still get a form to select the version you want, but Java 11 is pre-selected.

Link to adopt is ok, we should just mention that the user can also use another compatible jdk

Still don’t get, we also give openjdk for Ubuntu users, even though there are other distributions available. I changed it to

- Install Java, e.g. from [AdoptOpenJDK](https://adoptopenjdk.net/?variant=openjdk11) or any other distribution

Is that alright?

Co-Authored-By: Michael Kriese <[email protected]>

renovate-release · 2020-02-14T07:59:45Z

🎉 This PR is included in version 19.133.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

jGleitz force-pushed the document-java-for-local-development branch from ca93043 to b05f703 Compare February 11, 2020 09:37

JamieMagee reviewed Feb 11, 2020

View reviewed changes

docs/development/local-development.md Outdated Show resolved Hide resolved

jGleitz added 4 commits February 11, 2020 23:02

docs: document dependency on Java for local-development

ed60a2b

docs: recommend OpenJDK instead of Oracle’s JDK

bd035f1

docs: Java between 8 and *12* works

4449909

test(gradle): Skip tests for which the Java version is not sufficient

acacd2b

jGleitz force-pushed the document-java-for-local-development branch from bff9b7f to ecffddd Compare February 11, 2020 21:05

jGleitz added 2 commits February 11, 2020 23:30

test(gradle): don’t fail on missing Java, except on CI

b35efd5

docs: Java is no longer required to run tests

76dcb5a

jGleitz force-pushed the document-java-for-local-development branch from ecffddd to 6630284 Compare February 11, 2020 21:30

ci: Adapt Java skip logic

7f8002a

jGleitz force-pushed the document-java-for-local-development branch from 6630284 to 7f8002a Compare February 11, 2020 21:57

jGleitz force-pushed the document-java-for-local-development branch from 8fa3c7a to 1cbe07f Compare February 11, 2020 22:51

ci: Re-add SKIP_JAVA_TESTS environment variable

8f2ae61

jGleitz force-pushed the document-java-for-local-development branch from 1cbe07f to 8f2ae61 Compare February 11, 2020 22:53

rarkins requested a review from JamieMagee February 12, 2020 06:03

Merge branch 'master' into document-java-for-local-development

724f190

ci: Run Java tests on every OS, but only once

848ff27

viceice requested changes Feb 13, 2020

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

.github/workflows/test.yml Outdated Show resolved Hide resolved

docs/development/local-development.md Outdated Show resolved Hide resolved

viceice reviewed Feb 13, 2020

View reviewed changes

jGleitz and others added 3 commits February 13, 2020 21:14

ci: don’t run Java tests in CI on master

db1b940

docs: installing default-jre-headless is sufficient on Ubuntu

56eea39

Co-Authored-By: Michael Kriese <[email protected]>

docs: any Java distribution is okay

8e392ca

viceice approved these changes Feb 13, 2020

View reviewed changes

rarkins changed the title ~~Document Dependency on Java for Local Development~~ test: update and document java requirements for gradle Feb 13, 2020

rarkins merged commit 57108d4 into renovatebot:master Feb 13, 2020

renovate-release added the released label Feb 14, 2020

jGleitz deleted the document-java-for-local-development branch February 19, 2020 14:07

github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: update and document java requirements for gradle #5435

test: update and document java requirements for gradle #5435

jGleitz commented Feb 11, 2020 •

edited

Loading

JamieMagee Feb 11, 2020

rarkins Feb 11, 2020

rarkins Feb 11, 2020

jGleitz Feb 11, 2020

rarkins Feb 11, 2020

rarkins Feb 11, 2020

jGleitz Feb 11, 2020

rarkins Feb 11, 2020

jGleitz Feb 11, 2020 •

edited

Loading

jGleitz Feb 11, 2020

jGleitz commented Feb 11, 2020 •

edited

Loading

jGleitz commented Feb 11, 2020

rarkins commented Feb 12, 2020

viceice commented Feb 12, 2020

viceice commented Feb 12, 2020

viceice commented Feb 12, 2020

rarkins commented Feb 12, 2020

jGleitz commented Feb 12, 2020

viceice commented Feb 12, 2020

jGleitz commented Feb 12, 2020 •

edited

Loading

jGleitz commented Feb 12, 2020

viceice commented Feb 12, 2020

jGleitz commented Feb 13, 2020

viceice commented Feb 13, 2020

jGleitz commented Feb 13, 2020

viceice Feb 13, 2020

jGleitz Feb 13, 2020

viceice Feb 13, 2020

jGleitz Feb 13, 2020

renovate-release commented Feb 14, 2020

test: update and document java requirements for gradle #5435

test: update and document java requirements for gradle #5435

Conversation

jGleitz commented Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jGleitz Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jGleitz commented Feb 11, 2020 • edited Loading

jGleitz commented Feb 11, 2020

rarkins commented Feb 12, 2020

viceice commented Feb 12, 2020

viceice commented Feb 12, 2020

viceice commented Feb 12, 2020

rarkins commented Feb 12, 2020

jGleitz commented Feb 12, 2020

viceice commented Feb 12, 2020

jGleitz commented Feb 12, 2020 • edited Loading

jGleitz commented Feb 12, 2020

viceice commented Feb 12, 2020

jGleitz commented Feb 13, 2020

viceice commented Feb 13, 2020

jGleitz commented Feb 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renovate-release commented Feb 14, 2020

jGleitz commented Feb 11, 2020 •

edited

Loading

jGleitz Feb 11, 2020 •

edited

Loading

jGleitz commented Feb 11, 2020 •

edited

Loading

jGleitz commented Feb 12, 2020 •

edited

Loading