2.x class method visibility

[epeee]

Check List:

• Fixes test class/method field visibility #888 (at least partially)
• Unit tests : YES
• Javadoc with a code example (API only) : YES

a very first draft for class and method visibility.

e.g.

  @Test
  public void test1() {
    Assertions.assertThat(ArrayList.class).hasDeclaredPublicMethods("size", "grow", "add" , "fastRemove");
  }

would end up in:

java.lang.AssertionError: 
Expecting
  <java.util.ArrayList>
to have public methods:
  <["size", "grow", "add", "fastRemove"]>
but the following are not public:
  <{"fastRemove"="private", "grow"="private"}>

[joel-costigliola]

@epeee, just a quick comment (I haven't reviewed the PR yet) can you put to have declared public methods instead of to have public methods for hasDeclaredPublicMethods assertion error message ?

[joel-costigliola]

typo: fails

[joel-costigliola]

succeeds

[joel-costigliola]

reword: has the given methods. I use {@code for something that denotes a java element like {@code true} or {@code equals}

[joel-costigliola]

minor refactoring: I think we should rename methods to methodNames so that it is clear that we are not dealing with the Method type.

The example would be better with a superclass for MyClass having a public method and use it in the successful assertion example.

[joel-costigliola]

I think a better example would show a superclass for MyClass with a public method and use it in the failed assertion example (keep the example of the non existent method)

[joel-costigliola]

plural

[joel-costigliola]

same comment as before: introduce a superclass

[joel-costigliola]

suggestion: finish the error message with but was % and add actual's modifier value ?

[joel-costigliola]

how do you now in doAssertHasMethods that the error message is about declared methods and not methods ?

[epeee]

thx for pointing that out. You are right, this one was still missing but should be fixed now.

[joel-costigliola]

I believe you can combine if conditions

[joel-costigliola]

rename actualMethods to better express what this map contains (methodsModifer ?)

[joel-costigliola]

When no methods are given, the assertion should only pass if the actual class has not methods of the expected category (that is consistent with contains() assertion that succeeds it the actual iterable/array is empty).

This should be true whatever the category of expected methods is (public, declared, ...)

[epeee]

k, thx. I will add this one.

[epeee]

To sum it up:
When no methods are given

• hasPublicMethods will always fail because of methods inherit from Object
• hasDeclaredPublicMethods will pass if no public methods are declared in the given class
• hasDeclaredMethods will pass if no methods are declared in the given class
• hasMethods also will always fail because of methods inherit from Object

Am I right?

[epeee]

@joel-costigliola thx for the feedback. I have integrated your suggestions.

[joel-costigliola]

Thanks @epeee, I'm currently polishing the code, will be integrated soonish.

hasMethods only checks public methods, I believe this is an error as we have an hasPublicMethods assertion, have I missed something ?

[joel-costigliola]

Another thing, can you (in the future) rebase your branches instead of merging the main branch (2.x in our case) into yours ?
The reason is that it is easier for me to keep a linear history with one commit per feature (most of the time at least). I took care of this branch but that involved a lot a cherry pick and few conflicts to solve.

[epeee]

@joel-costigliola k, I will do so in the future (rebase).

ad hasMethods only checks public methods:
good catch, the isEmptyAndHasNoMethodsWithModifier check is currently done just for public methods (wrongly).

[joel-costigliola]

@epeee thanks for the additional commit.

I was trying to run the tests and found some were flaky because ShouldHaveMethods does not enforce a predictable order in the set of methods it receives, for example this test fails sometimes:

    thrown.expectAssertionError(shouldNotHaveMethods(actual, true,
             newLinkedHashSet("publicMethod", "privateMethod", "protectedMethod")));
    classes.assertHasDeclaredMethods(someInfo(), actual);

because the order of methods is not always the same (a LinkedHashSet ensure).

The fix is simple, use SortedSet instead of Set in ShouldNotHaveMethods (it will also sort methods alphabetically which is gonna be helpful for users).

One more thing, could you add some tests for ShouldNotHaveMethods, you can follow ShouldHaveFields_create_Test as an example.

One last thing (promised !) could you format the constructors in ShouldHaveMethods so that it goes to a new line after a %n like in ShouldHaveSuppressedException:

  private ShouldHaveSuppressedException(Throwable actual, Throwable expectedSuppressedException) {
    super("%n" +
          "Expecting:%n" +
          "  <%s>%n" +
          "to have a suppressed exception with the following type and message:%n" +
          "  <%s> / <%s>%n" +
          "but could not find any in actual's suppressed exceptions:%n" +
          "  <%s>.",
          actual, expectedSuppressedException.getClass().getName(), expectedSuppressedException.getMessage(),
          actual.getSuppressed());
  }

it makes the error message easier to read.

thanks !

[epeee]

@joel-costigliola thx for your feedback. I've already updated the pr.

[joel-costigliola]

Integrated thanks !

[joel-costigliola]

@epeee I think it would be better to align fields and methods assertions, I'm thinking of reducing methods assertions to:

• hasMethods(String... methodNames) : check methods (whatever visibility)
• hasPublicMethods (String... methodNames)` : check public accessible methods
• hasDeclaredMethods(String... methodNames) : check declared methods (includes non public ones)
  I want to do so to be consistent with the fields assertions
• hasPublicFields(String... fieldNames) : check public accessible fields
• hasDeclaredFields(String... fieldNames) : check declared fields (includes non public ones)

I might consider renaming hasMethods to hasPublicMethods and hasFields to hasPublicFields to avoid ambiguity.

WDYT ?

[epeee]

@joel-costigliola: 👍
actually, it looks like the javadoc for org.assertj.core.api.AbstractClassAssert#hasMethods is wrong (the sample using the private method).
edit: just realized that you already fixed the doc for 2.x 👍