Skip to content

Commit

Permalink
Harden painless test against "fun" caching (#24077)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nik9000 authored Apr 17, 2017
1 parent 34eda1a commit 25119a7
Showing 1 changed file with 19 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
package org.elasticsearch.painless;

/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
Expand All @@ -19,7 +17,11 @@
* under the License.
*/

import org.apache.lucene.util.Constants;
package org.elasticsearch.painless;

import org.elasticsearch.test.ESTestCase;

import static java.util.Collections.singletonMap;

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

public void testBranchEqualsDefAndPrimitive() {
assumeFalse("test fails on Windows", Constants.WINDOWS);
assertEquals(true, exec("def x = 1000; int y = 1000; return x == y;"));
assertEquals(false, exec("def x = 1000; int y = 1000; return x === y;"));
assertEquals(true, exec("def x = 1000; int y = 1000; return y == x;"));
assertEquals(false, exec("def x = 1000; int y = 1000; return y === x;"));
/* 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
* we can never be sure that the JVM hasn't configured itself to cache that Integer. It is sneaky like that. */
int uncachedAutoboxedInt = randomValueOtherThanMany(i -> Integer.valueOf(i) == Integer.valueOf(i), ESTestCase::randomInt);
assertEquals(true, exec("def x = params.i; int y = params.i; return x == y;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(false, exec("def x = params.i; int y = params.i; return x === y;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(true, exec("def x = params.i; int y = params.i; return y == x;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(false, exec("def x = params.i; int y = params.i; return y === x;", singletonMap("i", uncachedAutoboxedInt), true));
}

public void testBranchNotEquals() {
Expand All @@ -150,11 +154,13 @@ public void testBranchNotEquals() {
}

public void testBranchNotEqualsDefAndPrimitive() {
assumeFalse("test fails on Windows", Constants.WINDOWS);
assertEquals(false, exec("def x = 1000; int y = 1000; return x != y;"));
assertEquals(true, exec("def x = 1000; int y = 1000; return x !== y;"));
assertEquals(false, exec("def x = 1000; int y = 1000; return y != x;"));
assertEquals(true, exec("def x = 1000; int y = 1000; return y !== x;"));
/* 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
* we can never be sure that the JVM hasn't configured itself to cache that Integer. It is sneaky like that. */
int uncachedAutoboxedInt = randomValueOtherThanMany(i -> Integer.valueOf(i) == Integer.valueOf(i), ESTestCase::randomInt);
assertEquals(false, exec("def x = params.i; int y = params.i; return x != y;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(true, exec("def x = params.i; int y = params.i; return x !== y;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(false, exec("def x = params.i; int y = params.i; return y != x;", singletonMap("i", uncachedAutoboxedInt), true));
assertEquals(true, exec("def x = params.i; int y = params.i; return y !== x;", singletonMap("i", uncachedAutoboxedInt), true));
}

public void testRightHandNull() {
Expand Down

0 comments on commit 25119a7

Please sign in to comment.