From 25119a7e7822782989f0005de992708dfac54d6b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 17 Apr 2017 13:44:05 -0400 Subject: [PATCH] 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 --- .../elasticsearch/painless/EqualsTests.java | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/EqualsTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/EqualsTests.java index 16995f60dff..9045a390f2a 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/EqualsTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/EqualsTests.java @@ -1,5 +1,3 @@ -package org.elasticsearch.painless; - /* * Licensed to Elasticsearch under one or more contributor * license agreements. See the NOTICE file distributed with @@ -19,7 +17,11 @@ package org.elasticsearch.painless; * 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 { @@ -132,11 +134,13 @@ public class EqualsTests extends ScriptTestCase { } 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() { @@ -150,11 +154,13 @@ public class EqualsTests extends ScriptTestCase { } 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() {