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

Migrate test suite to end-to-end tests #35

Merged
merged 6 commits into from
May 27, 2020
Merged

Conversation

ggalmazor
Copy link
Contributor

@ggalmazor ggalmazor commented May 22, 2020

Fixes #34

This PR affects only to the tests codebase.

This PR introduces a new TestHttpServer class responsible for replaying HTTP fixture files and providing tools to make assertions about the HTTP request that this API client makes.

The biggest change this PR introduces is a change in the way we express expectations, by asserting through a RecordedRequest object. This is due to the limitations of running the JUnit assertions in the same thread that the server is running, which is not possible without putting an important amount of effort into it. As a consequence, testing has to be made following these general ideas:

  • DNSimpleBaseTest provides a server and a client instance and ensures that there's no interference between test methods.
  • Assertions about the request have to be made through the RecordedRequest object that server.getRecordedRequest() provides. This object includes the JSON payload, headers, path, and HTTP method information which allows any of the existing assertions to be made.
  • server.stubFixtureAt(String path) can be used to configure the server to replay a fixture file as the response to requests.
    For now, it's a basic stubbing system that will replay the same one fixture for all requests, but it could be easily extended to replay sequences of fixtures for multiple requests or specific fixtures per requested path or HTTP method.
  • In order to test exception throwing blocks, there's now a thrownException matcher that enables verifying the exception class and doing introspective testing as well. Example:
    @Test
    public void testDisableDnssecWhenNotEnabled() {
      server.stubFixtureAt("disableDnssec/not-enabled.http");
    
      assertThat(() -> client.domains.disableDnssec("1", "example.com"), allOf(
          thrownException(is(instanceOf(DnsimpleException.class))),
          thrownException(property(DnsimpleException::getStatusCode, is(428)))
      ));
    }
    DomainDnssecTest.testDisableDnssecWhenNotEnabled

We can't throw assertion errors without affecting the server's stability, which means we need to move the expectations outside the stubbing of the fixtures.

This commit does all changes necessary and adapts all tests by first setting the server's behavior and then providing means to ask about what's happened.

Everything related to the expected request that the API should have done is made through a new object of type RecordedRequest that the server provides. Everything related to the response must be tested through the actual output of the client call, which supports the idea of these tests being end-to-end.
@ggalmazor ggalmazor marked this pull request as ready for review May 26, 2020 08:08
@san983
Copy link
Member

san983 commented May 26, 2020

Did a quick first pass and looks good. Tests run ✅but wonder if we can keep them less verbose.

@weppos
Copy link
Member

weppos commented May 27, 2020

I am removing myself from review. I'll leave @san983 here. A second pair of eyes is probably not necessary, but if you need one maybe @jacegu or another team member with Java experience can assist.

@weppos weppos removed their request for review May 27, 2020 13:48
@ggalmazor ggalmazor requested a review from jacegu May 27, 2020 13:49
@ggalmazor ggalmazor mentioned this pull request May 27, 2020
@ggalmazor
Copy link
Contributor Author

Tests run ✅but wonder if we can keep them less verbose.

Just realized that I had some System.out.println calls leftover from when I was working on the test server. I've pushed a commit to get rid of them. Now you should only see assertion failures

Copy link
Member

@san983 san983 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After live-reviewingthis with @ggalmazor, this looks good 👍 ✅.

I raised some questions that can fit in the roadmap Guille is gonna draft. Thanks!!!

@ggalmazor ggalmazor merged commit 6c7a64d into master May 27, 2020
@ggalmazor ggalmazor deleted the task/34_migrate_test branch May 27, 2020 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate all tests to end-to-end integration tests
3 participants