Skip to content

Commit 313547b

Browse files
committed
Harden painless test against "fun" caching (#24077)
The JVM caches `Integer` objects. This is known. A test in Painless was relying on the JVM not caching the particular integer `1000`. It turns out that when you provide `-XX:+AggressiveOpts` the JVM *does* cache `1000`, causing the test to fail when that is specified. This replaces `1000` with a randomly selected integer that we test to make sure *isn't* cached by the JVM. *Hopefully* this test is good enough. It relies on the caching not changing in between when we check that the value isn't cached and when we run the painless code. The cache now is a simple array but there is nothing preventing it from changing. If it does change in a way that thwarts this test then the test fail fail again. At least when that happens the next person can see the comment about how it is important that the integer isn't cached and can follow that line of inquiry. Closes #24041
1 parent b0f87ca commit 313547b

File tree

1 file changed

+19
-13
lines changed

1 file changed

+19
-13
lines changed

modules/lang-painless/src/test/java/org/elasticsearch/painless/EqualsTests.java

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
package org.elasticsearch.painless;
2-
31
/*
42
* Licensed to Elasticsearch under one or more contributor
53
* license agreements. See the NOTICE file distributed with
@@ -19,7 +17,11 @@
1917
* under the License.
2018
*/
2119

22-
import org.apache.lucene.util.Constants;
20+
package org.elasticsearch.painless;
21+
22+
import org.elasticsearch.test.ESTestCase;
23+
24+
import static java.util.Collections.singletonMap;
2325

2426
// TODO: Figure out a way to test autobox caching properly from methods such as Integer.valueOf(int);
2527
public class EqualsTests extends ScriptTestCase {
@@ -132,11 +134,13 @@ public void testBranchEquals() {
132134
}
133135

134136
public void testBranchEqualsDefAndPrimitive() {
135-
assumeFalse("test fails on Windows", Constants.WINDOWS);
136-
assertEquals(true, exec("def x = 1000; int y = 1000; return x == y;"));
137-
assertEquals(false, exec("def x = 1000; int y = 1000; return x === y;"));
138-
assertEquals(true, exec("def x = 1000; int y = 1000; return y == x;"));
139-
assertEquals(false, exec("def x = 1000; int y = 1000; return y === x;"));
137+
/* This test needs an Integer that isn't cached by Integer.valueOf so we draw one randomly. We can't use any fixed integer because
138+
* we can never be sure that the JVM hasn't configured itself to cache that Integer. It is sneaky like that. */
139+
int uncachedAutoboxedInt = randomValueOtherThanMany(i -> Integer.valueOf(i) == Integer.valueOf(i), ESTestCase::randomInt);
140+
assertEquals(true, exec("def x = params.i; int y = params.i; return x == y;", singletonMap("i", uncachedAutoboxedInt), true));
141+
assertEquals(false, exec("def x = params.i; int y = params.i; return x === y;", singletonMap("i", uncachedAutoboxedInt), true));
142+
assertEquals(true, exec("def x = params.i; int y = params.i; return y == x;", singletonMap("i", uncachedAutoboxedInt), true));
143+
assertEquals(false, exec("def x = params.i; int y = params.i; return y === x;", singletonMap("i", uncachedAutoboxedInt), true));
140144
}
141145

142146
public void testBranchNotEquals() {
@@ -150,11 +154,13 @@ public void testBranchNotEquals() {
150154
}
151155

152156
public void testBranchNotEqualsDefAndPrimitive() {
153-
assumeFalse("test fails on Windows", Constants.WINDOWS);
154-
assertEquals(false, exec("def x = 1000; int y = 1000; return x != y;"));
155-
assertEquals(true, exec("def x = 1000; int y = 1000; return x !== y;"));
156-
assertEquals(false, exec("def x = 1000; int y = 1000; return y != x;"));
157-
assertEquals(true, exec("def x = 1000; int y = 1000; return y !== x;"));
157+
/* This test needs an Integer that isn't cached by Integer.valueOf so we draw one randomly. We can't use any fixed integer because
158+
* we can never be sure that the JVM hasn't configured itself to cache that Integer. It is sneaky like that. */
159+
int uncachedAutoboxedInt = randomValueOtherThanMany(i -> Integer.valueOf(i) == Integer.valueOf(i), ESTestCase::randomInt);
160+
assertEquals(false, exec("def x = params.i; int y = params.i; return x != y;", singletonMap("i", uncachedAutoboxedInt), true));
161+
assertEquals(true, exec("def x = params.i; int y = params.i; return x !== y;", singletonMap("i", uncachedAutoboxedInt), true));
162+
assertEquals(false, exec("def x = params.i; int y = params.i; return y != x;", singletonMap("i", uncachedAutoboxedInt), true));
163+
assertEquals(true, exec("def x = params.i; int y = params.i; return y !== x;", singletonMap("i", uncachedAutoboxedInt), true));
158164
}
159165

160166
public void testRightHandNull() {

0 commit comments

Comments
 (0)