-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added GetEnvironmentStrings to 'com.sun.jna.platform.win32.Kernel32' #434
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1758,13 +1758,14 @@ public EnumKey(HKEY hKey, int dwIndex) { | |
* @return A environment block | ||
*/ | ||
public static String getEnvironmentBlock(Map<String, String> environment) { | ||
StringBuilder out = new StringBuilder(); | ||
StringBuilder out = new StringBuilder(environment.size() * 32 /* some guess about average name=value length*/); | ||
for (Entry<String, String> entry : environment.entrySet()) { | ||
if (entry.getValue() != null) { | ||
out.append(entry.getKey() + "=" + entry.getValue() + "\0"); | ||
String key=entry.getKey(), value=entry.getValue(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Space/tab? Make sure there're spaces around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been the style of changes I submitted in the past and no one remarked about it. I have taken notice and will make an effort to have future submissions adhere to this style. As far as this submission goes, I would ask that you accept this change as-is, since back-formatting all the code I wrote with the spaces around the assignment operator would be tedious (I know Eclipse has a formatting option but I am afraid that other formatting issues will appear that may not be the way you like it - unless you can send me the exact configuration file for the Eclipse style). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I overlooked this before, it's not that important, but it looks sloppy to future developers - I'll take it, but I would appreciate if you could look into auto-formatting this and others (I don't have the exact Eclipse configuration, sorry), and maybe run something automatic for a future commit. We would like to make JNA look like it was written by 1 person. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duly noted (though I disagree with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if (value != null) { | ||
out.append(key).append("=").append(value).append('\0'); | ||
} | ||
} | ||
return out.toString() + "\0"; | ||
return out.append('\0').toString(); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* Copyright (c) 2007 Timothy Wall, All Rights Reserved | ||
* | ||
* This library is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public | ||
* License as published by the Free Software Foundation; either | ||
* version 2.1 of the License, or (at your option) any later version. | ||
* | ||
* This library is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
*/ | ||
package com.sun.jna.platform.win32; | ||
|
||
import java.util.Map; | ||
import java.util.Random; | ||
|
||
import org.junit.Test; | ||
|
||
import com.sun.jna.Native; | ||
|
||
/** | ||
* @author lgoldstein | ||
*/ | ||
public class Kernel32EnvironmentVarsTest extends AbstractWin32TestSupport { | ||
public Kernel32EnvironmentVarsTest() { | ||
super(); | ||
} | ||
|
||
@Test | ||
public void testKernelUtilGetEnvironmentStrings() { | ||
Map<String,String> vars=Kernel32Util.getEnvironmentVariables(); | ||
for (Map.Entry<String,String> entry : vars.entrySet()) { | ||
String name=entry.getKey(), expected=entry.getValue(); | ||
char[] data=new char[expected.length() + 1]; | ||
int size=Kernel32.INSTANCE.GetEnvironmentVariable(name, data, data.length); | ||
assertEquals("Mismatched retrieved length for " + name, data.length - 1 /* w/o the '\0' */, size); | ||
|
||
String actual=Native.toString(data); | ||
assertEquals("Mismatched retrieved value for " + name, expected, actual); | ||
} | ||
} | ||
|
||
@Test | ||
public void testKernelSetAndGetEnvironmentVariable() { | ||
String name=getCurrentTestName(), expected="42"; | ||
assertCallSucceeded("SetEnvironmentVariable", Kernel32.INSTANCE.SetEnvironmentVariable(name, expected)); | ||
|
||
try { | ||
int size = Kernel32.INSTANCE.GetEnvironmentVariable(name, null, 0); | ||
assertEquals("Mismatched required buffer size", expected.length() + 1, size); | ||
|
||
char[] data = new char[size]; | ||
assertEquals("Mismatched retrieved variable data length", size - 1, Kernel32.INSTANCE.GetEnvironmentVariable(name, data, size)); | ||
|
||
String actual=Native.toString(data); | ||
assertEquals("Mismatched retrieved variable value", expected, actual); | ||
} finally { | ||
assertCallSucceeded("Clean up variable", Kernel32.INSTANCE.SetEnvironmentVariable(name, null)); | ||
} | ||
} | ||
|
||
@Test | ||
public void testKernelUtilGetEnvironmentVariable() { | ||
String name=getCurrentTestName(), expected=Integer.toString(new Random().nextInt()); | ||
assertCallSucceeded("SetEnvironmentVariable", Kernel32.INSTANCE.SetEnvironmentVariable(name, expected)); | ||
try { | ||
assertEquals("Mismatched retrieved value", expected, Kernel32Util.getEnvironmentVariable(name)); | ||
} finally { | ||
assertCallSucceeded("Clean up variable", Kernel32.INSTANCE.SetEnvironmentVariable(name, null)); | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have split this into Kernel32EnvironmentTest and Kernel32UtilEnvironmentTest, but I'll accept it as is. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -539,21 +539,6 @@ public void testCreateProcess() { | |
assertTrue(processInformation.dwProcessId.longValue() > 0); | ||
} | ||
|
||
public void testGetEnvironmentVariable() { | ||
assertTrue(Kernel32.INSTANCE.SetEnvironmentVariable("jna-getenvironment-test", "42")); | ||
int size = Kernel32.INSTANCE.GetEnvironmentVariable("jna-getenvironment-test", null, 0); | ||
assertTrue(size == 3); | ||
char[] data = new char[size]; | ||
assertEquals(size - 1, Kernel32.INSTANCE.GetEnvironmentVariable("jna-getenvironment-test", data, size)); | ||
assertEquals(size - 1, Native.toString(data).length()); | ||
} | ||
|
||
public void testSetEnvironmentVariable() { | ||
int value = new Random().nextInt(); | ||
Kernel32.INSTANCE.SetEnvironmentVariable("jna-setenvironment-test", Integer.toString(value)); | ||
assertEquals(Integer.toString(value), Kernel32Util.getEnvironmentVariable("jna-setenvironment-test")); | ||
} | ||
|
||
public void testGetSetFileTime() throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are still functions in Kernel32, so they need tests by themselves, why did you remove them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see you moved them into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kernel32 is very big - to have a single class test all of the API seems too much - that is why in my past submissions (as in this one) I try to group tests related to a specific topic into a separate class (see NamedPipeTest) - as its the case for this submission - see EnvironmentVarsTest.. Regarding your remark, please note that I did not remove it (I would never remove a test unless some other test covers it) - I moved it to the EnvironmentVarsTest which groups the test for "excercising" the environment variables API(s) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to insist, don't take it personally :) Being very big hasn't been a problem. Being impossible to find or know what's tested and what is not has been a problem, we used to have "theme-based" tests and it created a huge mess where people were reimplementing tests twice that did the same thing in two different places. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that you want NamedPipeTest and WinconTest to be moved there as well ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do, although the PR will now contain more than what its name implies, so don't hold it against me... :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you commit to PRing ^^^ I will merge this as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m ok with breaking it up by functional group, e.g. Kernel32XXXTest.java (Kernel32EnvironmentVarsTest.java in this case). Just keep the prefix so it’s easy to see what belongs together.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On "don't hold it against me" I can recommend an excellent book. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will let you know in a few minutes when the change is made and pushed |
||
File tmp = File.createTempFile("testGetSetFileTime", "jna"); | ||
tmp.deleteOnExit(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think this is premature optimization, since I cannot think of a reason why to call this in a tight loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In hindsight I am inclined to agree (although not 100%...) - however, if it is not a critical issue for you, let's accept this for now and I will take notice in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not critical and I will take it, but I think you should really consider amending it.