On 8/28/13 9:44 AM, Erik Gahlin wrote:
Hi,
It took sometime, but here is an updated webrev where the shell script
has been modified.
http://cr.openjdk.java.net/~egahlin/7172176_3/
Looks good. Happy for you to get this pushed as it has been sitting in
your repo over a year :)
Mandy
Changes:
- added 'Darwin' to the shell script so it now also works on OS X.
- updated copyright to 2013 ;)
- changed from HashMap to IdentityHashMap, since same message can be
(and is) used by several keys
- removed test from problem list
Thanks
Erik
Erik Gahlin skrev 2012-06-08 21:59:
Mandy Chung skrev 2012-06-08 21:23:
Erik,
Tested in JPRT with "-testset core -onlytests '.*jdk_tools2.*'" on
all platforms,
Good. It didn't realize the previous webrev was not verified with
JPRT. It's always a good practice to run through shell test fix
with JPRT and sometimes you will find surprises :)
I ran it on the JPRT Stockholm queue and it doesn't have Macs (so I
excluded that platform, it was after all a Java fix) and of course it
was the only platform it failed on it.
Murphy's law ;)
Oh, well. I will update the shell script with "Darwin". If you are
okay without a new webrev, please say so and we can both save
ourselves some time :)
Otherwise I will post a new webrev on monday
Erik
http://cr.openjdk.java.net/~egahlin/7172176_02/
While I agree that a Java test is preferred to a shell test, I'd
prefer to keep this fix simple just to add Darwin just because I
don't have time to re-review this new fix. There are many shell
tests in the JDK that have been fixed to add Darwin but this one was
not updated because it was on the problem list.
FWIW. With jigsaw / modules, jconsole.jar is going away and your
test will fail and require fixing again; on the other hand, java -cp
jconsole.jar will work in a modular JDK for compatibility reason.
That's what will be coming for you to consider as well.
Mandy
On 6/7/2012 4:47 PM, Erik Gahlin wrote:
Hi again,
Could you please review an update?
Turns out that the ResourceCheck test didn't work for Mac when I
removed
it from the problems list. There was code in the shell script that
needed
to know all the platform names.
Instead of just adding "Darwin" to the script I decided to make the
test pure
Java, to speed things up and to avoid similar platform problems in the
future. I couldn't find a way in JTREG to add a test-JDK relative
library using
tags so I loaded the lib/jconsole.jar dynamically within the test.
Changes from previous webrev:
- classes will be loaded using a URLClassLoader
(test.jdk/lib/jconsole.jar)
- the resource bundle is loaded from the custom class loader
- the method Resources#getMnemonicInt is now invoked by reflection.
Not
super clean, but better then having a shell script in my opinion.
- removed failing tests from the ProblemsList.txt
Tested in JPRT with "-testset core -onlytests '.*jdk_tools2.*'" on
all platforms,
Here is the webrev:
http://cr.openjdk.java.net/~egahlin/7172176_02/
Thanks for taking the time to review this.
Erik
Erik Gahlin skrev 2012-05-30 13:40:
Hi,
Could you please review 7172176, which is a test fix for 7017818 -
JConsoleResources.java cannot be handled by translation team.
Webrev:
http://cr.openjdk.java.net/~egahlin/7172176_01/
Changes:
- Removed the ImmutableRequest.java and ImmutableRequest.sh since the
JConsoleResources class was refactored away with 7017818.
- Removed hard-wired resource bundle keys in
ResourceCheckTest.java, the keys
are now looked up by reflection on the Messages class. The test
also
checks there is one-to-one mapping between reource bundle keys and
the constants available in the Message class.
Michael,
I removed sun/tools/jconsole/ResourceCheckTest.sh from the jdk/test/
ProblemList.txt. This means the test will break once the
translated files
will be checked in. Unused messages were sent for translation and
they
need to be removed.
Are you ok with that?
Thanks!
Erik
