Skip to content

Conversation

TWiStErRob
Copy link
Contributor

@TWiStErRob TWiStErRob commented Jul 16, 2023

Re-raise of TWiStErRob#1 in mockito repo.

A step towards #2898, from here we could create multiple test tasks as a followup, and remove the gradle property and use the task name as dynamic matrix element instead.

Testing: see section in #3062

Additionally I added fail(System.getProperty("java.home")) in a test to see if the JDK was as expected.

I tried to minimize the changes, that's why I created a separate PR to make it possible to configure all projects at once.

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@TWiStErRob TWiStErRob changed the title Toolchains Toolchain for Test task Jul 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1fc4451) 85.45% compared to head (25958c5) 85.45%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #3064   +/-   ##
=========================================
  Coverage     85.45%   85.45%           
  Complexity     2888     2888           
=========================================
  Files           329      329           
  Lines          8801     8801           
  Branches       1093     1093           
=========================================
  Hits           7521     7521           
  Misses          992      992           
  Partials        288      288           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TimvdLippe
Copy link
Contributor

Do you know why the code coverage has increased with this PR? Looking at the changes, I don't see a particular reason why they would cover more. Or is this the result of running on Java 17 for Android? If so, are we also missing coverage now? I didn't spot anything, but let's double check.

Great change btw. We should definitely start using toolchains and now it is clear what we are using when.

@TWiStErRob
Copy link
Contributor Author

Do you know why the code coverage has increased with this PR? Looking at the changes, I don't see a particular reason why they would cover more.

@TimvdLippe the coverage changed because master android CI failed, so coverage from instrumentation tests was not included in baseline the PR is compared against. They passed on this PR, so they're covered, that's the increase. If you re-run this job on master so it passes, it should equalize. Please do.

@TWiStErRob TWiStErRob requested a review from TimvdLippe July 17, 2023 13:18
@TimvdLippe
Copy link
Contributor

@TWiStErRob can you rebase on master? I merged some Dependabot PRs and unfortunately they now clash. That should also fix the coverage issue, as I reran that job

Using a toolchain installed via auto-provisioning, but having no toolchain repositories configured. This behavior is deprecated. Consider defining toolchain download repositories, otherwise the build might fail in clean environments; see https://docs.gradle.org/8.2/userguide/toolchains.html#sub:download_repositories
@TWiStErRob
Copy link
Contributor Author

Clean rebase, only the trivial conflict with enterprise bump.

@TWiStErRob
Copy link
Contributor Author

TWiStErRob commented Jul 18, 2023

@TimvdLippe Coverage diff looks good. Do you agree with changes listed here? The biggest one is producing Java 11 bytecode on Java 17 for production code.

@TimvdLippe TimvdLippe merged commit cf8b820 into mockito:main Jul 19, 2023
@TWiStErRob TWiStErRob deleted the toolchains branch July 19, 2023 12:24
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.

3 participants