Simplify java assertions and add messages to all #1027

krk · 2024-10-15T11:08:20Z

Description

Improve Assert.java readability, maintainability and extensibility. Introduce a new isAsserted function which isLess etc. calls with a Comparison enum. Logging and assertion is done in a single place.

Motivation and context

Accept a message parameter in all Assert.java is* methods and move assertion checks to a single method.

How has this been tested?

make test
make test LOG_LEVEL=FINE

Sample fine logs:

FINE: isAsserted false, 'sampled all threads 1/4': 40814.0 > 0.0
FINE: isAsserted false, 'sampled all threads 2/4': 40812.0 > 0.0
FINE: isAsserted false, 'sampled all threads 3/4': 40791.0 > 0.0
FINE: isAsserted false, 'sampled all threads 4/4': 40791.0 > 0.0
FINE: isAsserted false, 'sharedDiff < 0.1': 0.0 < 0.1

Sample assertion failure:

java.lang.AssertionError: Expected 0.002 > 0.01: vtable stub > 0.01
at one.profiler.test.Assert.isAsserted(Assert.java:45)
at one.profiler.test.Assert.isGreater(Assert.java:54)
at test.recovery.RecoveryTests.numbers(RecoveryTests.java:42)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at one.profiler.test.Runner.run(Runner.java:151)
at one.profiler.test.Runner.run(Runner.java:167)
at one.profiler.test.Runner.main(Runner.java:216)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

apangin

Thanks, I like the idea. Please find my comments inline.

apangin · 2024-10-15T12:35:06Z

test/one/profiler/test/Assert.java

+
+enum Comparison {
+    GT, GTE, LT, LTE,
+}


Only one top-level class per file, please.
Make it a nested class or move to a separate file.

apangin · 2024-10-15T12:54:49Z

test/one/profiler/test/Assert.java

 public class Assert {
    private static final Logger log = Logger.getLogger(Assert.class.getName());
+    private static final Map<Comparison, String> operator = new HashMap<>();
+    private static final Map<Comparison, BiPredicate<Double, Double>> comparator = new HashMap<>();


No need for maps - just store them directly in enum's fields.
Alternatively, enum may itself implement BiPredicate.

apangin · 2024-10-15T13:03:11Z

test/test/jfr/JfrTests.java

-        Assert.isGreater(eventsCount.get("jdk.ExecutionSample"), 50);
-        Assert.isGreater(eventsCount.get("jdk.JavaMonitorEnter"), 50);
-        Assert.isGreater(eventsCount.get("jdk.ObjectAllocationInNewTLAB"), 50);
+        Assert.isGreater(eventsCount.get("jdk.ExecutionSample"), 50, "jdk.ExecutionSample > 50");


I'm not a fan of repeating assertion in a message every time - this is more of a boilerplate that does not add much value. It's ok to leave it without a message. Or, given that stacktrace already contains line numbers, we may extract a line from the source code and show it to a user. Not sure it's worth doing, though.

Added source parser, assertion failures looks like this now:

java.lang.AssertionError: Expected 0.13666666666666666 < 0.01 at test/test/recovery/RecoveryTests.java:42 :41 Output out = p.profile("-d 3 -e cpu -o collapsed"); :42 👉 Assert.isLess(out.ratio("vtable stub"), 0.01); :43 Assert.isGreater(out.ratio("Numbers.loop"), 0.8); at one.profiler.test.Assert.assertComparison(Assert.java:44) at one.profiler.test.Assert.isLess(Assert.java:81) at test.recovery.RecoveryTests.numbers(RecoveryTests.java:42)

before:

java.lang.AssertionError: Expected 0.13666666666666666 < 0.01 at one.profiler.test.Assert.assertComparison(Assert.java:44) at one.profiler.test.Assert.isLess(Assert.java:81) at test.recovery.RecoveryTests.numbers(RecoveryTests.java:42)

apangin · 2024-10-15T13:05:24Z

test/one/profiler/test/Assert.java


+
+enum Comparison {
+    GT, GTE, LT, LTE,


Add EQ for completeness?

This reverts commit 95133a0.

apangin · 2024-10-15T15:23:47Z

test/one/profiler/test/SourceLocator.java

@@ -0,0 +1,68 @@
+package one.profiler.test;


We badly need a linter for file headers.

Can we merge #1016 if I remove the trailing whitespace checker for now(it is scanning .class files and giving false results)

apangin · 2024-10-15T15:26:41Z

test/one/profiler/test/SourceLocator.java

+            String className = element.getClassName();
+
+            try {
+                Class<?> clazz = Class.forName(className);


Why do we need to instantiate a class just to convert its name to a path? Isn't className sufficient?

We already had fileName with extension, simplifying this.

apangin · 2024-10-15T15:28:05Z

test/one/profiler/test/SourceLocator.java

+    }
+
+    private static String getFilePathFromClassLoader(String fileName, Class<?> clazz) {
+        String packagePath = clazz.getPackage().getName().replace('.', File.separatorChar);


I'd just use / for better readability. It works even on Windows.

It also makes sense to cut everything after the last $ in case it's a nested class.

apangin · 2024-10-15T15:30:08Z

test/one/profiler/test/SourceLocator.java

+            return "Error reading source file: " + ex.getMessage();
+        }
+
+        return sourceCode.toString().trim();


Each line is already trimmed, isn't it?

This was to remove the last \n.

roy-soumadipta · 2024-10-15T15:34:02Z

test/one/profiler/test/SourceLocator.java

+            int startLine = Math.max(1, lineNumber - 1);
+            int endLine = lineNumber + 1;
+
+            // Read lines, and append the target lines (-1, 0, +1)


Do we need +1 line for assertions? The next assertion will not be executed.

Agree - I'll just leave one line with the original assertion for simplicity. We're not going to replace IDE anyway )

In case the assertion spans multiple lines. Will simplify to single line then.

roy-soumadipta · 2024-10-15T15:34:29Z

test/one/profiler/test/Assert.java

+    private static final Logger log = Logger.getLogger(Assert.class.getName());
+
+    private static void assertComparison(Comparison comparison, double left, double right, String message) {
+        boolean asserted = !comparison.comparator.test(left, right);


Isn't assertionFailed going to be a better name?

It didn't "fail" though, we are checking a value and we find it is "asserted". Here used as "asserted" vs. "unasserted". Can clarify further if necessary.

apangin · 2024-10-15T20:55:27Z

test/test/lock/LockTests.java

-        assert out.contains("sun/nio/ch/DatagramChannelImpl.send");
-    }
+   @Test(mainClass = DatagramTest.class, debugNonSafepoints = true) // Fails on Alpine
+   public void datagramSocketLock(TestProcess p) throws Exception {


What's happened with the indentation?

apangin · 2024-10-15T20:55:48Z

test/test/lock/LockTests.java

-        out = p.profile("-e lock -d 3 -o collapsed");
-        assert out.contains("sun/nio/ch/DatagramChannelImpl.send");
-    }
+   @Test(mainClass = DatagramTest.class, debugNonSafepoints = true) // Fails on Alpine


Btw, the test no longer fails on Alpine, the comment can be removed.

apangin · 2024-10-15T20:57:27Z

test/one/profiler/test/SourceLocator.java

+        sourceCode.append(filePath);
+        sourceCode.append(":");
+        sourceCode.append(lineNumber);
+        sourceCode.append("\n");


Since we return just one line, StringBuilder is redundant; use regular string concatenation or String.format.

apangin · 2024-10-15T20:57:54Z

test/one/profiler/test/SourceLocator.java

+
+            while ((line = reader.readLine()) != null) {
+                if (currentLine == lineNumber) {
+                    sourceCode.append(" 👉 ").append(line.trim());


Just return <string>;

apangin · 2024-10-15T21:02:37Z

test/one/profiler/test/SourceLocator.java

+    }
+
+    private static String getFilePathFromClassLoader(String className) {
+        int dollar = className.lastIndexOf("$");


nit: certain string methods (including lastIndexOf) have single-char variants - prefer them where possible.

apangin · 2024-10-15T21:05:07Z

test/one/profiler/test/SourceLocator.java

+        if (stackTrace.length > ignoreFrames) {
+            StackTraceElement element = stackTrace[ignoreFrames];
+            String className = element.getClassName();
+            String filePath = getFilePathFromClassLoader(className);


...FromClassName? ClassLoader is not involved here.

apangin · 2024-10-15T21:08:52Z

test/one/profiler/test/Assert.java

+        String operation = left + " " + comparison.operator + " " + right;
+
+        log.log(Level.FINE, "isAsserted " + asserted + ", "
+                + (message == null ? "" : "'" + message + "'") + ": "


A string won't look right when message == null, will it?

Punctuation still looks wrong:

isAsserted false, : 0 < 1 ^^^

apangin · 2024-10-15T21:14:50Z

test/one/profiler/test/Assert.java

+            String lines = "\n" + SourceLocator.tryGetFrom(new Exception(), 2);
+            String msg = message != null ? (": " + message + "\n" + lines) : lines;


Construction is a bit hard to read. There'll be an extra empty line if the message is not empty, is it intended?

Fixed in message null and not-null cases, simplified the construct.

apangin · 2024-10-16T12:53:25Z

test/one/profiler/test/Assert.java

+        String operation = left + " " + comparison.operator + " " + right;
+
+        log.log(Level.FINE, "isAsserted " + asserted + ", "
+                + (message == null ? "" : "'" + message + "'") + ": "


Punctuation still looks wrong:

isAsserted false, : 0 < 1 ^^^

apangin · 2024-10-16T15:32:09Z

test/one/profiler/test/SourceLocator.java

+
+            while ((line = reader.readLine()) != null) {
+                if (currentLine == lineNumber) {
+                    return result + "\n\t👉 " + line.trim();


Can you construct the entire string here? It will be easier to read comparing to when the first part of the message is somewhere else.
Also, let's avoid non-ASCII character. It does not make a big difference here, but who knows where it might go off.

I think it is a good idea to keep concerns separate, getting the source code line and building the assertion message are different responsibilities. Renamed methods and classes and further simplified Assert.java

krk added 2 commits October 15, 2024 11:05

Simplify how assertions are made and reported in Assert.java

dff1095

Add messages to all java test assertions.

95133a0

apangin reviewed Oct 15, 2024

View reviewed changes

apangin mentioned this pull request Oct 15, 2024

Reset CallTraceStorage counters before reporting live objects. #1009

Merged

krk added 3 commits October 15, 2024 15:14

address comments

05ae87c

address comments and add SourceLocator.

917a8de

Revert "Add messages to all java test assertions."

31fbf47

This reverts commit 95133a0.

apangin reviewed Oct 15, 2024

View reviewed changes

roy-soumadipta reviewed Oct 15, 2024

View reviewed changes

krk added 3 commits October 15, 2024 15:54

address comments

4e50b6b

fix build

88a3cd2

change format

1470c1c

apangin reviewed Oct 15, 2024

View reviewed changes

address comments

84dd779

apangin reviewed Oct 16, 2024

View reviewed changes

krk added 2 commits October 17, 2024 10:05

further simplify Assert.java

35ee641

Merge remote-tracking branch 'upstream/master' into simpleassert

4beee1c

apangin merged commit 62dca46 into async-profiler:master Oct 17, 2024
2 checks passed

		String lines = "\n" + SourceLocator.tryGetFrom(new Exception(), 2);
		String msg = message != null ? (": " + message + "\n" + lines) : lines;

Simplify java assertions and add messages to all #1027

Simplify java assertions and add messages to all #1027

Uh oh!

Conversation

krk commented Oct 15, 2024

Description

Motivation and context

How has this been tested?

Uh oh!

apangin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels