getappmap / appmap-java

AppMap client agent for Java
Other
80 stars 14 forks source link

Java test mapping can be disabled per-test via annotations #230

Closed brikelly closed 10 months ago

brikelly commented 11 months ago

This is important for our CI integration: users need to be able to disable tests that fail when used with AppMap, or if the test has unpredictable behavior that makes it continually cause changes to show up in unexpected places in our analysis report.

This should be done with a Java annotation, named something like AppMap:

import com.appland.appmap.annotation.AppMap;
...
@Test
@AppMap(false)
public void testFunction() {
  // test code here
}

Naming the annotation something like Disable is also a possibility, but that could make developers unfamiliar with AppMap think that the test is entirely disabled in JUnit.

For reference, the Ruby approach is as follows:

describe "performance testing", appmap: false do

  # This test will not be recorded
  it 'performs the action quickly' do
    # ...
  end

end
kgilpin commented 11 months ago

@apotterri do you feel like @AppMap(false) will be idiomatic for Java? Or is there more to it.

apotterri commented 11 months ago

@AppMap(false) seems good to me. JUnit 5 has an annotation called @Disabled, so I agree that we want to stay away from something like that.

kgilpin commented 11 months ago

Will @AppMap in a more specific context re enable mapping?

kgilpin commented 11 months ago

What about @NoAppMap

I kind of like the simplicity of not requiring an argument.

apotterri commented 10 months ago

Seems like we would want both @AppMap and @NoAppMap: @AppMap enables recording of a test, @NoAppMap disables it.

Will @AppMap in a more specific context re enable mapping?

I'd say so, yes. Those contexts would be, from least-specific to most-specific, package, class, and method. So the setting for the package would be overridden by the setting for class, would be overridden by the setting for the method.

For example, with package-info.java containing

@NoAppMap
package com.example;

import com.appland.appmap.annotation.NoAppMap;

and ExampleTest.java containing

@AppMap
public class ExampleTest.java {
  @Test
  public testWithRecording() {}

  @NoAppMap
  public testWithoutRecording() {}
}

testWithRecording would generate an AppMap, and testWithoutRecording would not.

brikelly commented 10 months ago

@apotterri I like the idea of the 2 annotations, that's very explicit and readable.

I think the prioritization model of multiple competing uses of @NoAppMap and @AppMap is fine, certainly enough to move ahead IMO.

kgilpin commented 10 months ago

But @AppMap isn't required to enable AppMaps of a test case, right?

I will say, in the wild I have only seen the need to disable AppMaps of a test case at the class or function level. I have not seen the need to enable and re-enable AppMap recording within more granular contexts etc.

The way you are proposing to do it is certainly sensible. I'm just not sure that more than @NoAppMap at the class or function level is needed, because that's all I've seen anyone need so far.

apotterri commented 10 months ago

But @AppMap isn't required to enable AppMaps of a test case, right?

Right, unless an enclosing scope (class, package) had @NoAppMap on it.

apotterri commented 10 months ago

I decided to go with only @NoAppMap on methods and classes for now. If users tell us they also want @AppMap, and/or the ability to annotate packages, we can revisit.

appland-release commented 10 months ago

:tada: This issue has been resolved in version 1.23.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: