Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

a more mockable api would be a nice to have #64

Open
deanhiller opened this issue Jul 20, 2018 · 6 comments
Open

a more mockable api would be a nice to have #64

deanhiller opened this issue Jul 20, 2018 · 6 comments

Comments

@deanhiller
Copy link

This may be worth reading before my below comment https://blog.twitter.com/engineering/en_us/topics/insights/2017/the-testing-renaissance.html

One gold standard for making an api that is testable is following these rules for an api

    • All requests passed into a method must be constructable by the client
    • All requests passed into a method should have no methods other than get/set (ie. no business logic, data only)
    • All responses returned from a method should be constructable by a test
    • All responses returned from a method should have no methods other than get/set (ie. no business logic)

I know it is a long shot, BUT it would rock if you released an api like that on top of your existing api.

Instead for now, I had to create an AcmeClientProxy which has a ton of untestable code due to the api of acme4j :(.

There are some things that can be considered just data like X509Cert, but there are way too many things like Order that have connect methods and such in them making it really confusing as to know when the library itself is going remote.

In fact, all twitter clients(100%) that go remote are

"public Dto method(Dto request)" making them testable AND very clear as to when it goes remote(ie. if I call the method, it goes remote).

thanks,
Dean

@shred
Copy link
Owner

shred commented Jul 21, 2018

Thank you for your feedback, Dean!

The DTO/DAO pattern is my favorite one for these kind of clients, too. And actually, acme4j before version 0.6 was designed like that.

However, the ACME protocol is somewhat special. There is a nonce that comes from the response of the previous request, and must be presented in the next one. Also the account's keypair must be present all the time, to sign the requests. So in every service method, besides the resource DTO, also a session and account object had to be passed in. Due to the nature of DTOs, it was also unclear what properties needed to be set, and what properties held an actual result when the service returned a DTO.

At the bottom line, the old API required too much background knowledge of the ACME protocol. It was difficult to use, felt very low level, and was prone to bugs. The only advantage probably was that it was easy to write unit tests for it. 😄

One major goal of acme4j is to present a concise API that helps the user to get a certificate, without needing to worry about all the ACME stuff happening in the background. This is why I decided to move to a classic domain model design in version 0.6.

But I agree that it is very difficult to write unit tests now. It shouldn't be like that.

I think putting a DTO/DAO API on top is not the right approach. As mentioned above, I have been there, and I didn't like the result at all.

The resource objects use the org.shredzone.acme4j.connector.Connection interface for communicating with the ACME server. So one way would be to write a MockConnection that emulates the communication with the server. This could be done by a MockProvider that is especially made for unit testing. One advantage is that, since you are using the actual acme4j service methods, you would get results from acme4j in your unit tests that are close to real world results.

What do you think about it?

@shred
Copy link
Owner

shred commented May 2, 2019

Now that the ACME protocol is finalized, I am actively working on this issue. There will be a mock framework available with the next acme4j version.

@shred
Copy link
Owner

shred commented Dec 7, 2019

Current status: The mock framework can already be used for simple tests that end in successful issuance of a certificate. I am currently working on extending the framework so it can also simulate all kind of error situations and "in progress" states. However, it will take some more time. 😞

I have removed the mock module from the master branch for now, and relocated it to the feature/mock branch. It was a mistake to add the module to the master branch in an early stage, as it was blocking continuous acme4j updates for too long now.

I will rebase the feature branch frequently, so the mock module won't fall behind the master branch. Feel free to use the mock module if it's okay for you that it is experimental.

@stefanofornari
Copy link

I have ran into this thread just now. I had a look at the mock branch, it may be some good stuff, but I think the current unit tests have an easier approach that I like more. At the end is just about stubbing the provider and the connection objects.
It can be generalized giving AcmeProviderStub a queue of responses for more complex scenarios like a full renew round-trip. I am doing it in my separate project but it would be great to create an artifact in here as per #162

@shred
Copy link
Owner

shred commented Sep 22, 2024

The mock branch grew very complex, as I tried to fully simulate a full mock CA. I also doubt that it can be rebased to the current master without having to solve many merge conflicts.

I think it would be a good approach to start with a simple acme4j-test module that just helps to build unit tests with a simple unit test test provider. The CA behavior could then be mocked by the tester, e.g. with the Mockito framework. Would that be helpful?

@stefanofornari
Copy link

yes very helpful. with a couple of flexible stubs (for the provider and connection) you do not even need to use mokito at all. The way I am doing it right now looks like the following:

Session session = new Session("acmetest:renew://cacert1.com");

based on the given uri the stub takes ownership, checks the subschema (renew above) and looks up for some configuration in the accept() method:

public boolean accepts(final URI uri) {
        if (uri == null) {
            return false;
        }
        final String scheme = uri.getScheme();

        if ((scheme == null) || !scheme.equals(SCHEME)) {
            return false;
        }

        //
        // Scheme is here accepted. If a subscheme is provided, it is used to
        // load this provider configuration from the resources
        //
        final String schemeSpecific = uri.getSchemeSpecificPart();
        final int pos = schemeSpecific.indexOf("://");
        if (pos > 0) {
            //
            // we have a subscheme
            //
            JSON stubs = TestUtils.getJSON("stubs/" + schemeSpecific.substring(0, pos));
                stubs.get("responseQueue").asArray().forEach((stub) -> {
                    responseQueue.offer(stub.asString());
            });
        }

        return true;
    }

The configuration is something like:

{
    "responseQueue": [
        "updateOrderResponse", "updateOrderResponse",
        "authorizationResponse1", "authorizationResponse2",
        "updateOrderResponse", "updateOrderResponseValid"
    ]
}

As you can see, for now it is just a queue of responses the provider will get from the connection.

Such configuration is then used to create a proper connection stub:

@Override
public Connection connect(URI serverUri, NetworkSettings networkSettings) {
    return new AcmeConnectionStub(resolve(serverUri), responseQueue.poll());
}

of course, this is very basic, it can be improved in many ways, but it should be enough to get the idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants