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

Allow overriding w32.ascii when running test suite #1231

Conversation

T-Svensson
Copy link
Contributor

Passing -Dw32.ascii=true or -Dw32.ascii=false on the command line to ANT
will forward the property to the test suite. This allows to run tests on
both the ANSI and the UNICODE versions of the API explicitly.
Not passing the -Dw32.ascii command line option results in the same
behavior as previous versions, i.e. the property is not set.

Signed-off-by: Torbjörn Svensson azoff@svenskalinuxforeningen.se

@dbwiddis
Copy link
Contributor

dbwiddis commented Jul 11, 2020

I don't think this is something we want. We're lying to the JNA code and saying the native code expects ANSI, but the native code hasn't changed. I would expect many things to break.

@T-Svensson
Copy link
Contributor Author

T-Svensson commented Jul 11, 2020

I don't think this is something we want. We're lying to the JNA code and saying the native code expects ANSI, but the native code hasn't changed. I would expect many things to break.

Ehm, no? We simply tell the JNA to use the A functions instead of the W functions when available.
This changeset only allows the test suite to be executed for ANSI or UNICODE(the default API variant if not set explicitly).

I don't see that this is any different from running an end-user application with -Dw32.ascii=true/false. Am I misunderstanding the purpose of the systemproperty?

@dbwiddis
Copy link
Contributor

I've retracted my earlier comment. You are correct that there's no difference between this an an application making that same definition. Which Microsoft advises new applications not to do. While I trust the -A functions with ASCII and UTF-8, I don't trust the OS as much on other character sets.

Anyway, I'm not familiar with ant, so I'll step back and let someone else review this PR.

@dblock
Copy link
Member

dblock commented Jul 14, 2020

Unless this is actually run by some CI automation, what is the purpose of allowing the test suite to execute in both ANSI and UNICODE?

@T-Svensson
Copy link
Contributor Author

Unless this is actually run by some CI automation, what is the purpose of allowing the test suite to execute in both ANSI and UNICODE?

There appears to be no CI running on Windows for the moment, but that does not mean that the test suite should not contain tests that cover all (or at least, most) of the execution paths.
In my case, I'm trying to fix the ANSI part of the mapping to actually work and it is a lot simpler to run the test suite using ANT locally than running each individual test case from within Eclipse or similar IDE.

In the best of worlds, I guess the JNA project should have a CI slave that would execute both versions of the API regularly to ensure that there is no breakage.

@dbwiddis
Copy link
Contributor

We do run CI tests on Windows using Appveyor, although it appears to be manually triggered rather than done on every pull request. I think that would be a good long-term goal, but shouldn't be switched on until all the failures it will cause are fixed.

@T-Svensson
Copy link
Contributor Author

We do run CI tests on Windows using Appveyor, although it appears to be manually triggered rather than done on every pull request. I think that would be a good long-term goal, but shouldn't be switched on until all the failures it will cause are fixed.

Okay, didn't know. Though you were only using Travis, but that's good news! :)

In any case, I agree with you that this should not be enabled until the test suite completes without any problems. With that said, should we block the possibility to run the suite at all manually with the option set?
I mean, if there is no easy way to run the test suite to discover the issues, how will they ever be fixed?

This changeset only introduces the possibility to pass the option to JUnit, it does not force you to do it, neither does it run with both or whatever... It will simply allow the one running ANT to decide rather than having JNA to always pick UNICODE.

So, to be clear; this does not introduce that ANSI tests are executed unless explicitly asked to do tests on the ANSI API by passing the -Dw32.ascii=true command line argument to ANT.

@dblock
Copy link
Member

dblock commented Jul 14, 2020

I'd prefer to see this merged with CI and all the fixes obviously, but I see your point now.

@dbwiddis
Copy link
Contributor

I favor merging this as an intermediate step to allow someone (probably @T-Svensson ) to go through and fix all the failures and then update the Appveyor script to run both tests. I'd also favor activating Appveyor on PRs, not sure how to configure that though. :)

I still want someone else to merge this as I don't know ant. It looks right to me, though.

@dbwiddis
Copy link
Contributor

While your other commits were mostly test improvements, this additional switch is deserving of a changelog entry. Can you kick off the 5.7.0 section and add this as a new feature?

Passing -Dw32.ascii=true or -Dw32.ascii=false on the command line to ANT
will forward the property to the test suite. This allows to run tests on
both the ANSI and the UNICODE versions of the API explicitly.
Not passing the -Dw32.ascii command line option results in the same
behavior as previous versions, i.e. the property is not set.

Signed-off-by: Torbjörn Svensson <azoff@svenskalinuxforeningen.se>
@T-Svensson

This comment has been minimized.

@T-Svensson T-Svensson force-pushed the iso/allow-override-w32.ascii-from-and-cmdline branch 2 times, most recently from 95aab8f to 8326059 Compare July 15, 2020 17:50
@matthiasblaesing
Copy link
Member

I'm going to merge this - the option to test is good.

@matthiasblaesing matthiasblaesing merged commit d8ce23e into java-native-access:master Jul 15, 2020
@T-Svensson T-Svensson deleted the iso/allow-override-w32.ascii-from-and-cmdline branch July 15, 2020 18:44
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.

4 participants