ls1intum / Ares

The Artemis Java Test Sandbox. A JUnit 5 Extension for Easy and Secure Artemis Java Testing
https://ls1intum.github.io/Ares/
MIT License
18 stars 7 forks source link

Allow access to (package) private and protected attributes, constructors, and methods #213

Closed b-fein closed 2 years ago

b-fein commented 2 years ago

What?

This PR adds additional methods to the ReflectionTestsUtils to allow access to (package) private and protected attributes, constructors, and methods.

Why?

Sometimes it might be useful to test for such non-accessible elements in classes. E.g.,

In most cases this new functionality should not be needed but it might sometimes be helpful for beginners to ‘overspecify’ the model in the task UML and then provide feedback based on the exact implementation of a specific design pattern.

Additional Context/Implementation Choices

MaisiKoleni commented 2 years ago

@krusche As I am not a frequent user of this class, would you like to have a look at the proposed API change as well, or do you know anyone else working with it that should? The additions are necessary (we had them in our courses as well a few times, just only in the exercise test repo), but the API design of the ReflectionTestUtils has reached its limits here. I don't think there is anything else we can do without breaking stable API compatibility.

MaisiKoleni commented 2 years ago

@krusche recommended @JohannesStoehr and @Strohgelaender as reviewers for this PR, that's why I added both of you here. Please wait with the reviews as long as this PR is in draft, though.

b-fein commented 2 years ago

I now had time to work on this PR again. It is now again ready to review.

I redesigned the search for inherited methods so that it is both compatible with the old way of finding methods/fields and searching for all additional protected inherited methods/fields. I’m now searching recursively in both superclasses and implemented interfaces. Existing tests remained unchanged, except one where the test fixture class changed slightly and a different method name now has the wrong access modifier. Therefore, I’m fairly confident that users of the existing methods for exclusively accessing accessible class members should see no changed behaviour for those. Multiple tests checking for intended (non-)access to direct members and members from superclasses and interface default ones have been added. With that the previously unused elements of the test fixture classes are now used.

The remaining comments about the documentation in ReflectionTestUtils should now be resolved as well.

MaisiKoleni commented 2 years ago

I will probably be able to do a full re-review this weekend. Thanks for fixing all the issues, I already feared you gave up on this PR, thank you for putting all the effort in 🥇

MaisiKoleni commented 2 years ago

The recent changes look very promising. I will test a few things later and get the other users to review it, too. Should be ready to merge, then.

MaisiKoleni commented 2 years ago

The code logic looks good. Just one remark about the naming, which might be confusing for tutors writing tests, and another comment about code style preferences

Regarding the naming, you only commented on the internal class and not on the public API. But I assume your thoughts include the ...Private methods in the ReflectionTestUtils?

JohannesStoehr commented 2 years ago

Regarding the naming, you only commented on the internal class and not on the public API. But I assume your thoughts include the ...Private methods in the ReflectionTestUtils?

Yes if the parameters are changed, I'd change the rest of the naming accordingly