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

Test/eKeiran #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Test/eKeiran #7

wants to merge 3 commits into from

Conversation

eKeiran
Copy link
Member

@eKeiran eKeiran commented Aug 15, 2023

Fixes #4
Tests working for the methods assigned to me:

  1. findByRegistration
  2. findByVisibility

Full coverage of methods in StudioRepository with a report for coverage generated by the JaCoCo plugin.

Copy link
Contributor

Choose a reason for hiding this comment

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

These images are not visible, what I would recommend is, put a one screenshot of the jacoco report as a comment in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

add unnecessary files in .gitignore, if .gitignore does not exists, make a new file in the root directory and add all unnecessary files into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain this change made in the assembler class, think of it this way: when I request findById() from the repository, Ids are unique so using a single object of Studio seems better than using list as there always going to be one object, it might be the case that I have missed out something so please update me if I missed out on anything

@@ -128,7 +128,7 @@ public List<StudiosDTO> getAllStudiosByUsers(long id){

public List<StudiosDTO> getAllStudiosByUsersFirstName(String name){
List<StudiosDTO> studiosDto = new ArrayList<>();
studioRepository.findByUsersSet_FirstName(name).forEach(studio -> studiosDto.add(getStudioMapper().studiosToStudiosDto(studio)));
studioRepository.findByName(name).forEach(studio -> studiosDto.add(getStudioMapper().studiosToStudiosDto(studio)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain this change? Also are you using the latest updates from the studio service repository?

public StudioRepository studioRepository;

@Test
public void testFindByRegistration() {
Copy link
Contributor

Choose a reason for hiding this comment

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

test looks good, just one addition, since it is a list, the recommended way is adding more than one object into the repository and then fetching them into the list and verify the size and properties of those studio object using assertEquals.

Copy link
Member

Choose a reason for hiding this comment

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

HTML test coverage should always be excluded from git commit.

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

Successfully merging this pull request may close these issues.

Test Coverage for Studio Repository
3 participants