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

Setup of test environment #34

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

Setup of test environment #34

wants to merge 7 commits into from

Conversation

briegema
Copy link
Collaborator

No description provided.

@@ -14,7 +16,7 @@
private final UserService userService;

@PostMapping("/sign-up")
public void signUp(@RequestBody ApplicationUser user) {
public void signUp(@RequestBody ApplicationUser user) throws IOException {
Copy link
Member

@elKei24 elKei24 Jun 10, 2020

Choose a reason for hiding this comment

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

I thought we do not use checked exceptions? 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as i remember there was a compilation problem and I made the quick fix. You are right we had this agreement, but then I ask myself why this compilation problem was on master?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I remember Intelij suggested to add it because of compilation problems. But why then was it necessary? I pulled from master?

I check this out

Comment on lines +27 to +41
ResultActions signUp = mockMvc.perform(
withJsonHeaders(post("/user/sign-up"))
.content(applicationUser.toString()));
ResultActions login = mockMvc.perform(
withJsonHeaders(post("/user/login"))
.content(applicationUser.toString()));

if (print) {
signUp.andDo(print());
login.andDo(print());
}

this.bearerToken = login.andReturn()
.getResponse()
.getHeader("Authorization");
Copy link
Member

Choose a reason for hiding this comment

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

Did you take a look at the link I sent you? Seems like there is a much more Spring-like aproach that lets us get rid of all this. Here is even a lot more about Security+Testing: https://docs.spring.io/spring-security/site/docs/5.3.2.RELEASE/reference/html5/#test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a short look at it, but i am more satisfied with reproducing the original requests one by one. Feels more like simply scripting API usage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had a short look.
I will check this out again


@Before
public void init() throws Exception {
DbCleaningHelper.truncateAllTables(dataSource.getConnection());
Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure using @Commit and manually trunkating the whole DB is bad style. Making unit tests depending on each other also is. Instead we should bring the DB in a useful state with proper dummy data either in a initialization function or even in a SQL file that spring loads when connecting to the h2 db (https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#howto-database-initialization).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I know this initialization only apply at the over all beginning of testing when the environment is set up. There is a way to reinitialize the db in this fashion before each test, but this is also very slow, because the whole environment would be set up again.

Comment on lines +9 to +16
abstract public class MockMvcHelper {

public static MockHttpServletRequestBuilder withJsonHeaders(MockHttpServletRequestBuilder builder) {
return builder.contentType(MediaType.APPLICATION_JSON)
.characterEncoding("utf-8");
}

}
Copy link
Member

Choose a reason for hiding this comment

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

There must be a more Spring style approch for doing that, probably something like:

@Component
public class ContentTypeHeaderMockMvcBuilderCustomizer implements MockMvcBuilderCustomizer {
    @Override
    public void customize(ConfigurableMockMvcBuilder<?> builder) {
        RequestBuilder improvedBuilder = MockMvcRequestBuilders.get("any")
                .contentType(MediaType.APPLICATION_JSON)
                .characterEncoding(StandardCharsets.UTF_8.name());
        builder.defaultRequest(improvedBuilder);
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@briegema briegema Jun 10, 2020

Choose a reason for hiding this comment

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

You are right and there are couples of other techniques. On the other side you wrote it to me yourself. Sometimes spring config stuff is complicated and especially it is not very domain specific. The whole utils stuff i wrote isn't in the spring style but it feasible to write down shortly the things we need.

However in this special case it might be worth to adapt your solution :)

@elKei24 elKei24 self-requested a review June 10, 2020 16:31
Copy link
Member

@elKei24 elKei24 left a comment

Choose a reason for hiding this comment

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

test

@elKei24 elKei24 self-requested a review June 10, 2020 16:31
Copy link
Member

@elKei24 elKei24 left a comment

Choose a reason for hiding this comment

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

test

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

Successfully merging this pull request may close these issues.

2 participants