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

Handle None being passed explicitly to startJVM #1134

Merged
merged 2 commits into from
May 19, 2023

Conversation

pelson
Copy link
Contributor

@pelson pelson commented May 2, 2023

Following-up with my comment in https://github.com/jpype-project/jpype/pull/1062/files#r1182683565.

Apologies to @marscher for missing this in #1062.

@pelson pelson force-pushed the fix/startup-args-handling branch from 90c77b9 to d847350 Compare May 2, 2023 15:39
@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #1134 (507ede7) into master (fffec42) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1134   +/-   ##
=======================================
  Coverage   87.84%   87.84%           
=======================================
  Files         112      112           
  Lines       10276    10276           
  Branches     4032     4032           
=======================================
  Hits         9027     9027           
  Misses        698      698           
  Partials      551      551           
Impacted Files Coverage Δ
jpype/_core.py 95.61% <100.00%> (ø)

@marscher
Copy link
Member

marscher commented May 2, 2023

Thanks, I've overlooked this one too. Do you think there are more spots like this? :)
Since this is a crucial entry point, we should probably create an extensive test matrix covering all combination and expected outcomes. I suggest using pytest.parametrize for this purpose.
Could you do that @pelson?

@pelson
Copy link
Contributor Author

pelson commented May 3, 2023

Do you think there are more spots like this? :)
Since this is a crucial entry point, we should probably create an extensive test matrix covering all combination and expected outcomes. I suggest using pytest.parametrize for this purpose.

Already the test_startup cases are pretty extensive. Naturally, as part of this work I extended those cases, and tried to be fairly systematic about checking things, but there can always be more things that slip through (particularly if undocumented). I'll take another look to see if there are any more obvious cases, but otherwise we can have reasonable confidence, and I'll try to address any issues quickly if they come up.


def testPathTwice(self):
with self.assertRaises(TypeError):
jpype.startJVM(self.jvmpath, jvmpath=self.jvmpath)

def testBadKeyword(self):
with self.assertRaises(TypeError):
jpype.startJVM(invalid=True)
jpype.startJVM(invalid=True) # type: ignore

Check failure

Code scanning / CodeQL

Wrong name for an argument in a call

Keyword argument 'invalid' is not a supported parameter name of [function startJVM](1).
@marscher marscher merged commit 8ed57b6 into jpype-project:master May 19, 2023
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