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

Enhance stability of tests and prepare for inclusion of appveyor #992

Merged

Conversation

matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Jul 30, 2018

Running the JNA unittests on the appveyor CI system shows, that implicit assumptions
in the unittests don't hold everywhere. There are:

  • corner cases in the behavior of Win32 functions
  • missing dependencies (several of the COM tests depend on one of the office products)
  • wrong invocations (the Win32DemoService picks up a system wide installed old JNA dll)
  • disabled service
    and so on.

The referenced commits either express the assumptions as junit assumption (causing the
affected tests to be skipped) or fix/improve the behavior.

Closes: #685

@matthiasblaesing
Copy link
Member Author

@dblock could you please have a look at this setup? I think I got everything tackled - my builds on appveyor run mostly clean, unittests on my development setup still run
@dbwiddis could you please also have a look? I modified the Pdh bindings: 14b746f as the Util method errored out on appveyor. I think the new version is more close to the original spirit of the base method and also matches what you initially implemented (using a result object, instead of a list of lists).

@dbwiddis
Copy link
Contributor

Looks good, although in my application, I remove the "_Total" instance from the list, so it doesn't work with the unmodifiable lists you've used so I'm having to change my iteration. I'm curious the reason to include that?

@dblock
Copy link
Member

dblock commented Jul 31, 2018

Looks great, I would add some CHANGELOG and consider @dbwiddis' comment above. Good to merge AFAIK @matthiasblaesing. Great work.

@dbwiddis
Copy link
Contributor

dbwiddis commented Jul 31, 2018

Another small issue. When attempting to use PdhEnumObjectItems with an invalid search string, I receive the error -1073738824 = 0xc0000bb8 (The specified object was not found on the system.) However, in the process throwing the Win32Exception with that value, it loses the value and ends up throwing a LastErrorException:

Exception in thread "main" com.sun.jna.LastErrorException: GetLastError() returned 317
at com.sun.jna.platform.win32.Kernel32Util.formatMessage(Kernel32Util.java:196)
at com.sun.jna.platform.win32.Kernel32Util.formatMessage(Kernel32Util.java:216)
at com.sun.jna.platform.win32.Win32Exception.(Win32Exception.java:69)
at com.sun.jna.platform.win32.Win32Exception.(Win32Exception.java:56)

The constructor for Win32Exception with an int implies it expects values in the range -32K to 32K (the lasterror values). I believe the error value may need a different / its own exception type. Happy to code that up in another PR if you would like.

@matthiasblaesing
Copy link
Member Author

@dbwiddis I agree, that the Win32Exception does not work for this usecase. I introduced a PdhException, that basicly just reports the error code. I also added a matching unittest.

For the modifiable lists: I have mixed feelings here. I like immutable objects, but I see your use case.

Please have another look:
2898fba

@dbwiddis
Copy link
Contributor

Looks good here.

@matthiasblaesing
Copy link
Member Author

@dblock @dbwiddis thank you both for having a look!

@matthiasblaesing matthiasblaesing merged commit 4441b3e into java-native-access:master Aug 1, 2018
@matthiasblaesing matthiasblaesing deleted the appveyor branch August 1, 2018 17:49
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

3 participants