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

Added some asserts on PetclinicIntegrationTests #344

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@jabrena
Copy link

commented Aug 12, 2018

I was reviewing the Project example Pet Clinic and I discovered that one test doesn´t include any assert so I added some asserts.

Juan Antonio

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Aug 12, 2018

@jabrena Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Aug 13, 2018

@jabrena Thank you for signing the Contributor License Agreement!

import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.*;

This comment has been minimized.

Copy link
@dsyer

dsyer Oct 5, 2018

Member

We never use "*" imports in Spring projects. Probably best to maintain that rule here.

This comment has been minimized.

Copy link
@jabrena

jabrena Oct 8, 2018

Author

I will follow that rule. I will try to change today.

@@ -45,4 +46,17 @@ public String toString() {
return this.getName();
}

@Override

This comment has been minimized.

Copy link
@dsyer

dsyer Oct 5, 2018

Member

Are these new methods actually used anywhere?

This comment has been minimized.

Copy link
@snicoll

snicoll Oct 7, 2018

Member

@dsyer the updated test relies indirectly on equals. IMO, I am with you that the implementation isn't good enough to show best practices so I am rather keen to decline this.

Having said that, the current test that this PR changes is problematic so we should do something about it.

This comment has been minimized.

Copy link
@jabrena

jabrena Oct 8, 2018

Author

(Sorry by the delay, I could't reply this weekend)

Yes, maybe the implementation is possible to be improved but conceptually the idea is right for every Spring Boot application. I will explain the idea: Any developer begin any Spring Boot app from scratch using Spring Initializr and later, the developer codes the project, for example Pet Clinic. When the developer implements Pet Clinic, the application should have some tests (Exist one) but without any assert (This is the essence of the Pull Request) I added some asserts and in order to run the test with the asserts I had to change some classes in the model in order to run the test. Maybe, It is possible to improve the syntax, probably yes, but the essence of the request is about every Test needs at least one assert. But I don't discuss the @snicoll criteria about the syntax. :)

@@ -76,4 +73,17 @@ public void addSpecialty(Specialty specialty) {
getSpecialtiesInternal().add(specialty);
}

@Override

This comment has been minimized.

Copy link
@dsyer

dsyer Oct 5, 2018

Member

I know that it can be a good idea to implement equals() and hashCode() for entities. Not sure these implementations are good enough to show best practice. Are they actually used in the app?

This comment has been minimized.

Copy link
@jabrena

jabrena Oct 8, 2018

Author

Hi @dsyer, for the tests was necessary. This was the reason. :)

I used the implementation from IntelliJ

@dsyer dsyer force-pushed the spring-projects:master branch from 9fe1647 to 912d2ad Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.