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

[ci] add jdks to test matrix #1131

Merged
merged 47 commits into from
Aug 22, 2024
Merged

[ci] add jdks to test matrix #1131

merged 47 commits into from
Aug 22, 2024

Conversation

marscher
Copy link
Member

@marscher marscher commented Apr 29, 2023

Added a mixture of JDK versions to our testing matrix. Version 8, 11, and 17 are being checked.

TODOs:

  • For some matrix elements the ivy dependencies are not installed (this leads to a failure in the test_sql*.py scripts)
  • Javadoc in JDK17 does not produce the same output as before.
  • Skip sql (custom drivers) tests for JDK8

@marscher marscher marked this pull request as ready for review April 29, 2023 11:46
@codecov
Copy link

codecov bot commented Apr 29, 2023

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.22%. Comparing base (d3cc970) to head (69bbe2e).
Report is 203 commits behind head on master.

Files with missing lines Patch % Lines
jpype/_jvmfinder.py 96.29% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
- Coverage   87.31%   87.22%   -0.09%     
==========================================
  Files         113      113              
  Lines       10281    10281              
  Branches     4065     4065              
==========================================
- Hits         8977     8968       -9     
- Misses        707      718      +11     
+ Partials      597      595       -2     
Files with missing lines Coverage Δ
jpype/_jvmfinder.py 61.53% <96.29%> (-7.70%) ⬇️

... and 2 files with indirect coverage changes

---- 🚨 Try these New Features:

@marscher marscher added this to the 1.5 milestone Apr 29, 2023
@marscher
Copy link
Member Author

@Thrameos I could use some help to figure out why the ivy deps are not found for some jobs - thank you :)

@Thrameos
Copy link
Contributor

Thrameos commented May 4, 2023

Sure. Though there may be other ways to skin that cat like calling it once and shipping the artifacts arounf. Been a while since I looked at it.

@Thrameos
Copy link
Contributor

Thrameos commented May 4, 2023

I looked through the logs. It is not an ivy failure. The first step of the process is to create the artifact deps which are then used for all builds. Thus if the failure was with ivy it would have failed in the first stage of the build and every build after it would have failed as well. The fact that some worked and some did not indicates that the dependencies are there but they didn't work for every build.

So lets look at the possibilities:

  1. The artifact failed to download and unpack properly. No evidence as the state says it was successful. Best way to debug is to add an "ls" or equivalent to download stage to see if everything was unpacked.
  2. The artifact unpacked but the jars did not work for the version of the JDK tested. This matches the behavior. (Though it took me looking at the patch as the JDK versions were random to the Python versions. We should add the JDK name to the description to make it clear.)

The jars being pulled are too new for 8 so most likely it is JDK 52.0 version conflict (causing one type of fail) and something has changed in 17 that is not allowing the driver to load in 17. The last one is more of a mystery as I rarely get things to fail forward unless they are hitting the ByteBuffer symbol conflict bug.

To diagnose I will have to install the same versions of JDK and the jars and try to get them to manually load in hopes of forcing an error. The JDK silently ignores any jars that fail to load from the classpath, so manual loading is required to get diagnostics. I will need to investigate further.

Safest fix for the JDK 8 is to add an option to skip the dbc tests for those builds. JDK is dead as a doornail at this point and it is hard to ensure that 3rd party jars will work with it going forward. 17 needs investigation.

@marscher
Copy link
Member Author

marscher commented May 4, 2023

thanks for looking into it. I'll be on vacation next week. So I wont be available that time. Do you intend to push the release in the next two weeks?

@Thrameos
Copy link
Contributor

Thrameos commented May 4, 2023

I was aiming for the end of April but I can push it back. We have a lot of things left to resolve so the time may be needed anyway.

jpype/_jvmfinder.py Fixed Show fixed Hide fixed
@Thrameos
Copy link
Contributor

Thrameos commented Aug 20, 2024 via email

@marscher
Copy link
Member Author

Right, I'm gonna finish this one soon.

test/jpypetest/test_sql_sqlite.py Fixed Show fixed Hide fixed
test/jpypetest/test_sql_sqlite.py Fixed Show fixed Hide fixed
test/jpypetest/test_sql_sqlite.py Fixed Show fixed Hide fixed
Comment on lines +153 to +154
"import jpype; jpype.startJVM(); "
"print(jpype.java.lang.System.getProperty('java.version'))"]),

Check warning

Code scanning / CodeQL

Implicit string concatenation in a list Warning test

Implicit string concatenation. Maybe missing a comma?
test/jpypetest/test_sql_sqlite.py Fixed Show fixed Hide fixed
java = jpype.java
import pytest

import common

Check notice

Code scanning / CodeQL

Module is imported with 'import' and 'import from' Note test

Module 'common' is imported with both 'import' and 'import from'.
test/jpypetest/test_sql_sqlite.py Dismissed Show dismissed Hide dismissed
jpype/_jvmfinder.py Dismissed Show dismissed Hide dismissed
@marscher marscher merged commit 66f8c6c into master Aug 22, 2024
26 of 27 checks passed
@marscher marscher deleted the ci_jdk_matrix branch August 22, 2024 02:58
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.

2 participants