-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add address retrieve API #24
base: main
Are you sure you want to change the base?
Conversation
I want to start by saying that we are not expecting any additional commits. Feel free to make them if you wish, but that's not an expectation that we have. With that said I will go through your changes and leave technical comments on them. |
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public interface AddressRepository extends JpaRepository<Address, String>, JpaSpecificationExecutor<Address> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JpaSpecificationExecutor
is not really required here since we're only querying on a single input.
List<Address> findByUserId(final String userId);
https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#repositories.core-concepts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah that's right. I'm rusty there it seems. Last time I had to add a method to a repository interface was in 2015, or 7 years ago
import javax.persistence.criteria.CriteriaQuery; | ||
import javax.persistence.criteria.Root; | ||
|
||
@RequiredArgsConstructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using @RequiredArgsConstructor
instead of @AllArgsConstructor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just use it by default, not much of a difference here I don't think. In case some one comes along and forgets to make their fields final
maybe this can sorta catch that.
.userId(Set.of(userId)) | ||
.build(); | ||
AddressSpecification specification = new AddressSpecification(filter); | ||
List<Address> addresses = addressRepository.findAll(specification); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userId
, specification
and addresses
can be final
. Do you know the benefits of making these variables final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I used to have that generator box checked too, but nowadays I think if someone have bugs from reassigning variables, then they probably have a bigger problem. Generally variables don't ever get reassigned anyway so there's no need to have final
all over the place. I think just having them on property fields should be enough, maybe occasionally parameters if it's some logic heavy method.
@Autowired | ||
private MockMvc mockMvc; | ||
@MockBean | ||
AddressService service; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service
should be private
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
} | ||
|
||
@Test | ||
void noDataShouldReturnEmptyList() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These test names are not very clear. It would quickly get confusing if more endpoints were added to the AddressController
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I typically use subjectMethodNameShouldBehaveThisWay as a pattern, so in this case it's enough, and yes if more endpoints get in here then it'll have to be updated.
@Mock | ||
private AddressRepository repository; | ||
@Autowired | ||
private ResourceMapper mapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SpringBootTest
is unncessary here.- You can use
@InjectMocks
to initializeAddressService
. - You should use a better name to refer to the
AddressService
variable. It's unclear whattestObj
is without looking at the variable type. - Mock the
ResourceMapper
. We don't wantResourceMapper
bugs to cause failures inAddressServiceTest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes a field gets mocked but don't get called in a when()
, then intellij says that variable is unused, which is not true but it messes with you when you're reading it.
With the mapper I think it being a utility component it's fine to have additional coverage on it, also gets rid of the need to do another when()
I call all subjectUnderTest testObj
so that way it's quick and easy to see what method is the test about. I usually middle click on it to bring the cursor to the type, or by looking at the test file name or the class name.
https://stackoverflow.com/questions/7321407/what-is-sut-and-where-did-it-come-from
Keeping the test object name consistent makes it easy to spot in any test classes. I have all my tests' SUT named testObj, and the reason for having all tests using the same test object name is because all the tests always have the same purpose of running the SUT method and check results. The business logic varies, but tests' purpose and flow stay the same.
when(repository.findAll(any(Specification.class))).thenReturn(List.of(address)); | ||
List<AddressDto> results = testObj.findAllByUserId(""); | ||
assertEquals(zip, results.get(0).getZip()); | ||
verify(repository).findAll(any(Specification.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should not verify
with concepts like any()
since the implementation can become flawed and your test won't catch that (because you're mocking the findAll
. Instead you should only use any()
in your when
statements and use the actual objects in your verify
statements. If you do this then you can be less specific in your when
statements.
final String userId = "";
// mock data
when(repository.findAll(any()).thenReturn(List.of(address));
final List<AddressDto> results = addressService.findAllByUserId(userId);
// asserts
final AddressFilter filter = new AddressFilter(Set.of(userId));
final AddressSpecification specification = new AddressSpecification(filter);
verify(repository).findAll(specification);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Sometimes I tack it on the back of assertions just to make sure the method gets called but I guess it's not very useful in this case.
Address Retrieve API should return all data for a given User: