Add a Reflection proxy utility class #2053

leocck · 2021-11-02T22:25:05Z

Stubs with Reflection

While solving the basic concept called "Cook your Lasagna", I noticed the tests would not run until I had implemented all the methods > reason being, the tests were calling the methods from the class directly, causing a build error.

To make the code compile, we should create stubs for the methods the students will implement, but the Lasagna exercise is about how to define methods, and by defining the methods in stubs we spoil the exercise somewhat.

This PR adds a utility class called ReflectionProxy, which simplifies the use of Reflection API by encapsulating the target class. With it, it is easy to have proxied stubs and check if the methods were implemented correctly at the same time.

This utility class was just copied over from source code we use at the ReDI School of Digital Integration with our students and might deserve some improvements. Here's an example of the exercises we are using it.

Reviewer Resources:

Track Policies

The proxy encapsulates the target class and helps checking if the methods were implemented correctly

leocck · 2021-11-02T22:52:05Z

exercises/concept/lasagna/src/test/java/utils/Lasagna.java

+        try {
+            return invokeMethod("expectedMinutesInOven", new Class[]{});
+        } catch (Exception e) {
+            return 0;


Here you will probably want to throw a UnsupportedOperationException, but this way also works

I've seen all the implementations of Reflection Proxies throw UnsupportedOperationException as you mentioned.

leocck · 2021-11-02T22:54:47Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+
+import static java.lang.Class.forName;
+
+public abstract class ReflectionProxy {


This class is long because features were added from time to time, in a collaborative but unorganised way.
I made my best to document all the methods before bringing it to this repository, but I'm sure you will find a lot of things that could be improved.

iHiD · 2021-11-03T10:59:09Z

This is a really interesting approach. Thanks. I'll let the Java maintainers review it etc, but just wanted to say thanks for the PR :)

ericbalawejder · 2021-11-03T20:06:26Z

I like the idea of testing for the correct method signature without the build failure. This is more of an issue when using the browser editor rather than local tools which was introduced in v3.

As you implemented the Lasagna class for the new tests, the tests pass that check the method stub. Should we create stubs that cause all the tests to fail? Such as omitting the access modifier and changing the return type to void? This would be more consistent with the TDD approach for building each problem.

leocck · 2021-11-03T20:26:59Z

I like the idea of testing for the correct method signature without the build failure. This is more of an issue when using the browser editor rather than local tools which was introduced in v3.

As you implemented the Lasagna class for the new tests, the tests pass that check the method stub. Should we create stubs that cause all the tests to fail? Such as omitting the access modifier and changing the return type to void? This would be more consistent with the TDD approach for building each problem.

I think the idea of this exercise is to provide them an empty class, so that they implement the whole method (signature and logic) by themselves. If we provide them wrong code, then I think the exercise description will have to be changed to "fix the provided code" or something like that. WDYT?

Just to see if I understood you correctly: you mean the Lasagna class they will have to implement (the template), right? The one from the util package is just a proxy (if the methods do not exist in the target class, or have the wrong signature, the tests will still fail).

ericbalawejder · 2021-11-03T20:51:01Z

Just to see if I understood you correctly: you mean the Lasagna class they will have to implement (the template), right? The one from the util package is just a proxy (if the methods do not exist in the target class, or have the wrong signature, the tests will still fail).

The idea is to provide a template for the student as such?

public class Lasagna extends ReflectionProxy {

    // Implement methods here

    @Override
    public String getTargetClassName() {
        return null;
    }

}

leocck · 2021-11-03T20:56:57Z

The idea is to provide a template for the student as such?

No, that's not the place. The students will get an empty class, from the src/main folder (this file).

The one you are referring to is inside the src/test folder and is used by the tests as a Proxy (it will try to call the one that the students must implement and check if they did it correctly) 😉

I know, it's a bit confusing because both classes have the same name (even though they are in different packages), but that was intended, so when the students look at the unit test code (to understand why it is failing) they could still understand which logic is being tested (without polluting the code with a lot of Reflection calls).

Does it make more sense now?

ericbalawejder · 2021-11-04T01:50:05Z

Yes, I had the path mixed up as you pointed out. I understand it as unit test --> proxy --> collaborator where the collaborator is the empty file src/main/java/Lasagna?

I like the idea and implementation.

ericbalawejder · 2021-11-04T01:54:04Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+     * An instance of the target class (if found)
+     */
+    private Object target;
+


target can be final for this implementation.

ericbalawejder · 2021-11-04T01:56:47Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+    public ReflectionProxy(Object target) {
+        this.target = target;
+    }
+


It doesn't technically matter for instantiation but would it be more valuable to change the constructors to protected to control scope?

Were there two constructors?

The constructor that receives the target instance is not being used. Moved to the "unused" region 👍

Ok, the main constructor is still marked public.

I think that's ok, since the class is abstract, right?

It doesn't technically matter for instantiation but would it be more valuable to change the constructors to protected to control scope?

It's more of a style thing since this may be used as a template in further exercises.

ericbalawejder · 2021-11-04T02:03:36Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+package utils;
+
+import java.lang.reflect.*;
+


import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.ParameterizedType;

It helps future maintainers when we avoid wildcard imports.

ericbalawejder · 2021-11-04T02:07:34Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+            return false;
+        }
+        try {
+            Constructor c = targetClass.getDeclaredConstructor(parameterTypes);


Constructor<?> c = targetClass.getDeclaredConstructor(parameterTypes);

Wildcard to avoid raw types?

ericbalawejder · 2021-11-04T02:09:32Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+        if (targetClass == null) {
+            return null;
+        }
+        Constructor[] constructors = getAllConstructors();


Constructor<?>[] constructors = getAllConstructors();

Wild card type?

ericbalawejder · 2021-11-04T02:11:57Z

exercises/concept/lasagna/src/test/java/utils/ReflectionProxy.java

+            return false;
+        }
+    }
+}


Not all the methods are used but it seems like a good template for future use in other exercises.

Yeah, I thought we could leave them for reference.

I suggest to comment the unused methods with a explicit message like Unused at moment or something like that.
It can improve the time to understand on the code for a maintainer who unknows it

I agree. What do you say about grouping the unused methods with a region section? (it works for almost all editors and is still "understandable" in case it is not supported)

It might be a good idea to create a Gradle package that contains the reflection proxy, as it can then be re-used across exercises. We've done a similar thing with C#: https://github.com/exercism/dotnet-tests

@ErikSchierboom : that's exactly what I wanted to do, but I don't know how to setup an internal Gradle package 😅 . If you point me in the right direction I will do it 🙏

We have to ask @iHiD to create a repository. Ideas on a good name?

Oh, ok, I thought we could move this class to an upper folder and be able to share it with all exercises.

Naming is the most difficult part 😅 . Are there any other utility libraries to refer to, or conventions already?

If not, I would suggest something like exercism:test-utils

I don't think we have any conventions. We don't yet support shared files between exercises and it might be nice to hide those files from the student anyway.

So, should we merge this PR and create another ticket for the new repository? (are you using Jira or other issue tracking?)

ericbalawejder · 2021-11-04T02:20:16Z

exercises/concept/lasagna/src/test/java/LasagnaTest.java

    @Test
    public void expected_minutes_in_oven() {
        assertThat(new Lasagna().expectedMinutesInOven()).isEqualTo(40);
    }

-    @Ignore("Remove to run test")
+    @Test


I believe the test runner actually removes the @Ignore annotation when it runs the tests on the Exercism server. The @Ignore annotation is normally placed on every test except the first one for local development purposes, i.e. exercism download --exercise=lasagna --track=java with the command line tool.

Interesting, I didn't know that, but it makes a lot of sense now

leocck · 2021-11-04T03:46:20Z

I like the idea and implementation.

Yay! Thanks for your review, I fixed all the issues you pointed out.

ErikSchierboom · 2021-11-04T11:11:37Z

Great idea! I'm planning to do something similar for the C# track.

ericbalawejder · 2021-11-04T22:58:53Z

@mirkoperillo The build action: Compile check style with gradle fails and has 65K+ lines of output. I assume it's running all of the exercises? Is this normal or intended behavior?

mirkoperillo · 2021-11-06T07:11:55Z

@ericbalawejder you have right. Currently when the CI workflow starts the gradle check command is executed on all the exercises. We need to improve it

java/.github/workflows/gradle.yml

Line 23 in acfdd2b

    
           run: cd exercises && ../gradlew check compileStarterSourceJava --parallel --continue --info

mirkoperillo

@leocck sorry for the late feedback, your PR is alright: I run the tests against it and work fine.
Currently the code breaks some style constraints.
Can I ask to run

gradle clean check

on your PR code and fix them ? Thank you a lot

leocck · 2021-11-16T10:31:43Z

@mirkoperillo thanks for looking into that, i always forget about the gradle check 🤦🏼

Fixed now

mirkoperillo · 2021-11-16T13:08:16Z

@leocck thank you, now PR is ready to be merged

Add a Reflexion proxy

61b07bc

The proxy encapsulates the target class and helps checking if the methods were implemented correctly

leocck commented Nov 2, 2021

View reviewed changes

ericbalawejder reviewed Nov 4, 2021

View reviewed changes

Add changes from PR review

ddf943b

leocck added 3 commits November 4, 2021 16:37

Group unused methods with region

8695511

Move unused constructor to "unused" region

a3f3ef6

Set constructor as protected

fa5e4c8

ericbalawejder approved these changes Nov 4, 2021 •

edited

Loading

View reviewed changes

mirkoperillo suggested changes Nov 16, 2021

View reviewed changes

Fix code style constraints

9e05aa7

mirkoperillo approved these changes Nov 16, 2021

View reviewed changes

mirkoperillo merged commit f95f76d into exercism:main Nov 16, 2021


		import static java.lang.Class.forName;

		public abstract class ReflectionProxy {

Uh oh!

Add a Reflection proxy utility class #2053

Add a Reflection proxy utility class #2053

Uh oh!

Conversation

leocck commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stubs with Reflection

Uh oh!

leocck Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

iHiD commented Nov 3, 2021

Uh oh!

ericbalawejder commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leocck commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericbalawejder commented Nov 3, 2021

Uh oh!

leocck commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericbalawejder commented Nov 4, 2021

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!

leocck Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

leocck commented Nov 2, 2021 •

edited

Loading

leocck Nov 2, 2021 •

edited

Loading

ericbalawejder commented Nov 3, 2021 •

edited

Loading

leocck commented Nov 3, 2021 •

edited

Loading

leocck commented Nov 3, 2021 •

edited

Loading

leocck Nov 4, 2021 •

edited

Loading