Skip to content

Commit 407e061

Browse files
amitgalitzamitgalitz
authored
Changed forbiddenAPIsTest files and made relevant forbidden fixes (opensearch-project#450)
Signed-off-by: Amit Galitzky <amgalitz@amazon.com>
1 parent 9dcf010 commit 407e061

28 files changed

+171
-94
lines changed

‎build.gradle

+4-6
Original file line numberDiff line numberDiff line change
@@ -131,12 +131,10 @@ tasks.named('forbiddenApisMain').configure {
131131
signaturesFiles += files('src/forbidden/ad-signatures.txt')
132132
}
133133

134-
tasks.named('forbiddenApisTest').configure {
135-
// Disable check because AD code has too many violations.
136-
// For example, we have to allow @Test to be used in test classes not inherited from LuceneTestCase.
137-
// see https://github.com/elastic/elasticsearch/blob/master/buildSrc/src/main/resources/forbidden/es-test-signatures.txt
138-
ignoreFailures = true
139-
}
134+
//Adds custom file that only has some of the checks for testApis checks since too many violations
135+
//For example, we have to allow @Test to be used in test classes not inherited from LuceneTestCase.
136+
//example: warning for every file: `Forbidden annotation use: org.junit.Test [defaultMessage Just name your test method testFooBar]`
137+
forbiddenApisTest.setSignaturesFiles(files('src/forbidden/ad-test-signatures.txt'))
140138

141139
// Allow test cases to be named Tests without having to be inherited from LuceneTestCase.
142140
// see https://github.com/elastic/elasticsearch/blob/323f312bbc829a63056a79ebe45adced5099f6e6/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/TestingConventionsTasks.java

‎src/forbidden/ad-test-signatures.txt

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
@defaultMessage use format with Locale
7+
java.lang.String#format(java.lang.String,java.lang.Object[])
8+
java.util.concurrent.ThreadLocalRandom
9+
10+
java.security.MessageDigest#clone() @ use org.opensearch.common.hash.MessageDigests
11+
12+
@defaultMessage Don't use MethodHandles in slow or lenient ways, use invokeExact instead.
13+
java.lang.invoke.MethodHandle#invoke(java.lang.Object[])
14+
java.lang.invoke.MethodHandle#invokeWithArguments(java.lang.Object[])
15+
java.lang.invoke.MethodHandle#invokeWithArguments(java.util.List)
16+
java.lang.Math#random() @ Use one of the various randomization methods from LuceneTestCase or OpenSearchTestCase for reproducibility

‎src/test/java/org/opensearch/action/admin/indices/mapping/get/IndexAnomalyDetectorActionHandlerTests.java

+20-8
Original file line numberDiff line numberDiff line change
@@ -264,7 +264,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
264264
listener.onResponse((Response) response);
265265
}
266266
} catch (IOException e) {
267-
e.printStackTrace();
267+
logger.error("Create field mapping threw an exception", e);
268268
}
269269
}
270270
};
@@ -345,7 +345,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
345345
listener.onResponse((Response) response);
346346
}
347347
} catch (IOException e) {
348-
e.printStackTrace();
348+
logger.error("Create field mapping threw an exception", e);
349349
}
350350
}
351351
};
@@ -437,7 +437,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
437437
listener.onResponse((Response) response);
438438
}
439439
} catch (IOException e) {
440-
e.printStackTrace();
440+
logger.error("Create field mapping threw an exception", e);
441441
}
442442
}
443443
};
@@ -527,7 +527,7 @@ public <Request extends ActionRequest, Response extends ActionResponse> void doE
527527
listener.onResponse((Response) response);
528528
}
529529
} catch (IOException e) {
530-
e.printStackTrace();
530+
logger.error("Create field mapping threw an exception", e);
531531
}
532532
}
533533
};
@@ -599,7 +599,10 @@ public void testTenMultiEntityDetectorsUpdateSingleEntityAdToMulti() throws IOEx
599599

600600
doAnswer(invocation -> {
601601
Object[] args = invocation.getArguments();
602-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
602+
assertTrue(
603+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
604+
args.length == 2
605+
);
603606

604607
assertTrue(args[0] instanceof SearchRequest);
605608
assertTrue(args[1] instanceof ActionListener);
@@ -613,7 +616,10 @@ public void testTenMultiEntityDetectorsUpdateSingleEntityAdToMulti() throws IOEx
613616

614617
doAnswer(invocation -> {
615618
Object[] args = invocation.getArguments();
616-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
619+
assertTrue(
620+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
621+
args.length == 2
622+
);
617623

618624
assertTrue(args[0] instanceof GetRequest);
619625
assertTrue(args[1] instanceof ActionListener);
@@ -675,7 +681,10 @@ public void testTenMultiEntityDetectorsUpdateExistingMultiEntityAd() throws IOEx
675681

676682
doAnswer(invocation -> {
677683
Object[] args = invocation.getArguments();
678-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
684+
assertTrue(
685+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
686+
args.length == 2
687+
);
679688

680689
assertTrue(args[0] instanceof SearchRequest);
681690
assertTrue(args[1] instanceof ActionListener);
@@ -689,7 +698,10 @@ public void testTenMultiEntityDetectorsUpdateExistingMultiEntityAd() throws IOEx
689698

690699
doAnswer(invocation -> {
691700
Object[] args = invocation.getArguments();
692-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 2);
701+
assertTrue(
702+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
703+
args.length == 2
704+
);
693705

694706
assertTrue(args[0] instanceof GetRequest);
695707
assertTrue(args[1] instanceof ActionListener);

‎src/test/java/org/opensearch/action/admin/indices/mapping/get/ValidateAnomalyDetectorActionHandlerTests.java

-29
Original file line numberDiff line numberDiff line change
@@ -137,38 +137,9 @@ public void testValidateMoreThanThousandSingleEntityDetectorLimit() throws IOExc
137137

138138
// extend NodeClient since its execute method is final and mockito does not allow to mock final methods
139139
// we can also use spy to overstep the final methods
140-
141140
NodeClient client = IndexAnomalyDetectorActionHandlerTests
142141
.getCustomNodeClient(detectorResponse, userIndexResponse, singleEntityDetector, threadPool);
143142

144-
// NodeClient client = new NodeClient(Settings.EMPTY, threadPool) {
145-
// @Override
146-
// public <Request extends ActionRequest, Response extends ActionResponse> void doExecute(
147-
// ActionType<Response> action,
148-
// Request request,
149-
// ActionListener<Response> listener
150-
// ) {
151-
// try {
152-
// if (action.equals(SearchAction.INSTANCE)) {
153-
// assertTrue(request instanceof SearchRequest);
154-
// SearchRequest searchRequest = (SearchRequest) request;
155-
// if (searchRequest.indices()[0].equals(ANOMALY_DETECTORS_INDEX)) {
156-
// listener.onResponse((Response) detectorResponse);
157-
// } else {
158-
// listener.onResponse((Response) userIndexResponse);
159-
// }
160-
// } else {
161-
// GetFieldMappingsResponse response = new GetFieldMappingsResponse(
162-
// TestHelpers.createFieldMappings(detector.getIndices().get(0), "timestamp", "date")
163-
// );
164-
// listener.onResponse((Response) response);
165-
// }
166-
// } catch (IOException e) {
167-
// e.printStackTrace();
168-
// }
169-
// }
170-
// };
171-
172143
NodeClient clientSpy = spy(client);
173144

174145
handler = new ValidateAnomalyDetectorActionHandler(

‎src/test/java/org/opensearch/ad/AnomalyDetectorJobRunnerTests.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.util.Arrays;
3030
import java.util.Collections;
3131
import java.util.Iterator;
32+
import java.util.Locale;
3233
import java.util.concurrent.ExecutorService;
3334
import java.util.concurrent.ThreadFactory;
3435

@@ -174,7 +175,10 @@ public void setup() throws Exception {
174175

175176
doAnswer(invocation -> {
176177
Object[] args = invocation.getArguments();
177-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
178+
assertTrue(
179+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
180+
args.length >= 2
181+
);
178182

179183
IndexRequest request = null;
180184
ActionListener<IndexResponse> listener = null;

‎src/test/java/org/opensearch/ad/AnomalyDetectorRestTestCase.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import java.time.Instant;
1919
import java.util.ArrayList;
2020
import java.util.Collections;
21+
import java.util.Locale;
2122
import java.util.Map;
2223

2324
import org.apache.http.HttpHeaders;
@@ -196,7 +197,7 @@ protected Response previewAnomalyDetector(String detectorId, RestClient client,
196197
.makeRequest(
197198
client,
198199
"POST",
199-
String.format(TestHelpers.AD_BASE_PREVIEW_URI, input.getDetectorId()),
200+
String.format(Locale.ROOT, TestHelpers.AD_BASE_PREVIEW_URI, input.getDetectorId()),
200201
ImmutableMap.of(),
201202
TestHelpers.toHttpEntity(input),
202203
null

‎src/test/java/org/opensearch/ad/HistoricalAnalysisRestTestCase.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ protected List<Object> waitUntilTaskReachState(String detectorId, Set<String> ta
253253
try {
254254
adTaskProfile = getADTaskProfile(detectorId);
255255
} catch (Exception e) {
256-
e.printStackTrace();
256+
logger.error("failed to get ADTaskProfile", e);
257257
} finally {
258258
Thread.sleep(1000);
259259
}

‎src/test/java/org/opensearch/ad/NodeStateManagerTests.java

+9-2
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.util.Arrays;
2929
import java.util.Collections;
3030
import java.util.HashSet;
31+
import java.util.Locale;
3132
import java.util.Set;
3233
import java.util.concurrent.CountDownLatch;
3334
import java.util.concurrent.TimeUnit;
@@ -147,7 +148,10 @@ private String setupDetector() throws IOException {
147148

148149
doAnswer(invocation -> {
149150
Object[] args = invocation.getArguments();
150-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
151+
assertTrue(
152+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
153+
args.length >= 2
154+
);
151155

152156
GetRequest request = null;
153157
ActionListener<GetResponse> listener = null;
@@ -175,7 +179,10 @@ private void setupCheckpoint(boolean responseExists) throws IOException {
175179

176180
doAnswer(invocation -> {
177181
Object[] args = invocation.getArguments();
178-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length >= 2);
182+
assertTrue(
183+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
184+
args.length >= 2
185+
);
179186

180187
GetRequest request = null;
181188
ActionListener<GetResponse> listener = null;

‎src/test/java/org/opensearch/ad/cluster/DailyCronTests.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.time.Clock;
2121
import java.time.Duration;
2222
import java.util.Arrays;
23+
import java.util.Locale;
2324

2425
import org.opensearch.OpenSearchException;
2526
import org.opensearch.action.ActionListener;
@@ -57,7 +58,10 @@ private void templateDailyCron(DailyCronTestExecutionMode mode) {
5758

5859
doAnswer(invocation -> {
5960
Object[] args = invocation.getArguments();
60-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 3);
61+
assertTrue(
62+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
63+
args.length == 3
64+
);
6165
assertTrue(args[2] instanceof ActionListener);
6266

6367
ActionListener<BulkByScrollResponse> listener = (ActionListener<BulkByScrollResponse>) args[2];

‎src/test/java/org/opensearch/ad/cluster/HashRingTests.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public void testGetOwningNode() throws UnknownHostException {
147147
// Circles for realtime AD will change as it's eligible to build for when its empty
148148
assertEquals("Wrong hash ring size for realtime AD", 2, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
149149
}, e -> {
150-
e.printStackTrace();
150+
logger.error("building hash ring failed", e);
151151
assertFalse("Build hash ring failed", true);
152152
}));
153153

@@ -166,7 +166,8 @@ public void testGetOwningNode() throws UnknownHostException {
166166
// Circles for realtime AD will not change as it's eligible to rebuild
167167
assertEquals("Wrong hash ring size for realtime AD", 2, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
168168
}, e -> {
169-
e.printStackTrace();
169+
logger.error("building hash ring failed", e);
170+
170171
assertFalse("Build hash ring failed", true);
171172
}));
172173

@@ -185,7 +186,7 @@ public void testGetOwningNode() throws UnknownHostException {
185186
);
186187
assertEquals("Wrong hash ring size for realtime AD", 4, hashRing.getNodesWithSameAdVersion(Version.V_1_1_0, true).size());
187188
}, e -> {
188-
e.printStackTrace();
189+
logger.error("building hash ring failed", e);
189190
assertFalse("Failed to build hash ring", true);
190191
}));
191192
}

‎src/test/java/org/opensearch/ad/cluster/HourlyCronTests.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.util.Arrays;
2121
import java.util.Collections;
2222
import java.util.HashMap;
23+
import java.util.Locale;
2324

2425
import org.apache.logging.log4j.LogManager;
2526
import org.apache.logging.log4j.Logger;
@@ -64,7 +65,10 @@ public void templateHourlyCron(HourlyCronTestExecutionMode mode) {
6465
Client client = mock(Client.class);
6566
doAnswer(invocation -> {
6667
Object[] args = invocation.getArguments();
67-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 3);
68+
assertTrue(
69+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
70+
args.length == 3
71+
);
6872
assertTrue(args[2] instanceof ActionListener);
6973

7074
ActionListener<CronResponse> listener = (ActionListener<CronResponse>) args[2];

‎src/test/java/org/opensearch/ad/cluster/MasterEventListenerTests.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.time.Clock;
2323
import java.util.Arrays;
2424
import java.util.HashMap;
25+
import java.util.Locale;
2526

2627
import org.junit.Before;
2728
import org.opensearch.ad.AbstractADTest;
@@ -83,7 +84,10 @@ public void testOnOffMaster() {
8384
public void testBeforeStop() {
8485
doAnswer(invocation -> {
8586
Object[] args = invocation.getArguments();
86-
assertTrue(String.format("The size of args is %d. Its content is %s", args.length, Arrays.toString(args)), args.length == 1);
87+
assertTrue(
88+
String.format(Locale.ROOT, "The size of args is %d. Its content is %s", args.length, Arrays.toString(args)),
89+
args.length == 1
90+
);
8791

8892
LifecycleListener listener = null;
8993
if (args[0] instanceof LifecycleListener) {

0 commit comments

Comments
 (0)