Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

Schema integration test #1 #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AndrewOlenik
Copy link
Contributor

Schema integration test #1

Two tests added.

  1. Does e2e test, i.e. goes to https://api.icndb.com
  2. Does the same but acts against mocked ICNDB client

1) Does e2e test, i.e. goes to https://api.icndb.com
2) Does the same but acts against mocked ICNDB client
@SpringBootTest
@RunWith(SpringRunner.class)
@AutoConfigureMockMvc
public class GraphQLIntegrationTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually an e2e test, please rename accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

how can I ensure that e2e tests are not executed in the same test run as unit and integration tests?

private static final String JOKE_TEXT = "Chuck Norris can divide by zero.";

@Autowired
private MockMvc mockMvc;
Copy link
Contributor

Choose a reason for hiding this comment

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

By going through MockMvc we fail to test that Spring's DispatcherServlet routes to GraphQLServlet correctly in a real world scenario i.e. when an app is deployed in to a servlet container. I believe that we should do an actual POST instead

Copy link
Contributor

@trioletas trioletas Apr 16, 2018

Choose a reason for hiding this comment

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

please see this comment for a visual representation re e2e testing scope

@SpringBootTest
@RunWith(SpringRunner.class)
@AutoConfigureMockMvc
public class GraphQLQueryTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually an integration test, please rename accordingly


private static final String GRAPHQL_PATH = "/graphql";

private static final String QUERY = "{jokes {jokeById(id: \"77\"){id,text}}}";
Copy link
Contributor

Choose a reason for hiding this comment

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

please extract to a .graphql file:
a) better IDE support, e.g. the IntelliJ GraphQL plugin will recognize and help out with the syntax
b) better readability for a human person

result.andExpect(status().isOk())
.andExpect(jsonPath("$.errors").doesNotExist())
.andExpect(jsonPath("$.data.jokes.jokeById.id").value(JOKE_ID))
.andExpect(jsonPath("$.data.jokes.jokeById.text").value(JOKE_TEXT));
Copy link
Contributor

Choose a reason for hiding this comment

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

in case of an error, $.errors will be set to an array of errors. in order to fail fast, please assert that the error array is empty

}

@SneakyThrows
private ResultActions graphQLPost() {
Copy link
Contributor

Choose a reason for hiding this comment

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

the fact that you actually do a HTTP POST is an implementation detail and is completely irrelevant to the reader, you may do a GET and nothing would change contract-wise.
please change the name to something like executeGraphQLQuery

private static final String JOKE_TEXT = "Chuck Norris can divide by zero.";

@Autowired
private AppProperties appProperties;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not auto wire required app properties directly? i.e.

 @Value("icndb.url") private String icndbUrl;

after all we only need a single value from the props, let's be straight about it

import java.util.List;

@Getter
@Setter
public class ICNDBJoke {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Getter and @Setter can be folded in to @Data

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

Successfully merging this pull request may close these issues.

None yet

2 participants