Fix member repo tests - #102
Conversation
|
| void updateMember_updatesEveryField() { | ||
| final Member originalMember = member; | ||
| final Member updatedMember = Member.builder() | ||
| .id(originalMember.getId()) | ||
| .firstName("Taylor") | ||
| .lastName("Reed") | ||
| .email("updated-" + originalMember.getEmail()) | ||
| .linkedInUrl("https://linkedin.com/in/taylor") | ||
| .introduction("Updated introduction") | ||
| .referralSource("Patina event") | ||
| .active(false) | ||
| .matchPref("Mentor") | ||
| .industryPref("Finance") | ||
| .rolePref("Product Manager") | ||
| .topics("Leadership") | ||
| .extraNotes("Available on weekends") | ||
| .build(); | ||
|
|
||
| final Member result = memberRepo.updateMember(updatedMember).orElseThrow(); | ||
|
|
||
| assertMemberFields(result, updatedMember); | ||
| assertNotNull(result.getCreatedAt()); | ||
| assertNotNull(result.getUpdatedAt()); | ||
| member = memberRepo.updateMember(originalMember).orElseThrow(); | ||
| } |
There was a problem hiding this comment.
Test pollution issue: The test mutates the shared member instance variable and attempts to restore it at line 137. If any assertion fails before line 137 (lines 134-136), the member will remain in its updated state, causing subsequent tests that depend on the original member data to fail.
Fix: Use @BeforeEach and @AfterEach instead of @BeforeAll and @AfterAll, or restore the member state in a try-finally block:
@Test
void updateMember_updatesEveryField() {
final Member originalMember = member;
final Member updatedMember = Member.builder()
.id(originalMember.getId())
// ... other fields
.build();
try {
final Member result = memberRepo.updateMember(updatedMember).orElseThrow();
assertMemberFields(result, updatedMember);
assertNotNull(result.getCreatedAt());
assertNotNull(result.getUpdatedAt());
} finally {
member = memberRepo.updateMember(originalMember).orElseThrow();
}
}| void updateMember_updatesEveryField() { | |
| final Member originalMember = member; | |
| final Member updatedMember = Member.builder() | |
| .id(originalMember.getId()) | |
| .firstName("Taylor") | |
| .lastName("Reed") | |
| .email("updated-" + originalMember.getEmail()) | |
| .linkedInUrl("https://linkedin.com/in/taylor") | |
| .introduction("Updated introduction") | |
| .referralSource("Patina event") | |
| .active(false) | |
| .matchPref("Mentor") | |
| .industryPref("Finance") | |
| .rolePref("Product Manager") | |
| .topics("Leadership") | |
| .extraNotes("Available on weekends") | |
| .build(); | |
| final Member result = memberRepo.updateMember(updatedMember).orElseThrow(); | |
| assertMemberFields(result, updatedMember); | |
| assertNotNull(result.getCreatedAt()); | |
| assertNotNull(result.getUpdatedAt()); | |
| member = memberRepo.updateMember(originalMember).orElseThrow(); | |
| } | |
| void updateMember_updatesEveryField() { | |
| final Member originalMember = member; | |
| final Member updatedMember = Member.builder() | |
| .id(originalMember.getId()) | |
| .firstName("Taylor") | |
| .lastName("Reed") | |
| .email("updated-" + originalMember.getEmail()) | |
| .linkedInUrl("https://linkedin.com/in/taylor") | |
| .introduction("Updated introduction") | |
| .referralSource("Patina event") | |
| .active(false) | |
| .matchPref("Mentor") | |
| .industryPref("Finance") | |
| .rolePref("Product Manager") | |
| .topics("Leadership") | |
| .extraNotes("Available on weekends") | |
| .build(); | |
| try { | |
| final Member result = memberRepo.updateMember(updatedMember).orElseThrow(); | |
| assertMemberFields(result, updatedMember); | |
| assertNotNull(result.getCreatedAt()); | |
| assertNotNull(result.getUpdatedAt()); | |
| } finally { | |
| member = memberRepo.updateMember(originalMember).orElseThrow(); | |
| } | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| void updateMember_updatesEveryField() { | ||
| final Member originalMember = member; | ||
| final Member updatedMember = Member.builder() | ||
| .id(originalMember.getId()) |
There was a problem hiding this comment.
should have a test for only updating some fields (or only a single field)
| public static final String GET_MEMBER_BY_ID_SQL = "SELECT * FROM members WHERE id = :id"; | ||
| public static final String GET_MEMBER_BY_EMAIL_SQL = "SELECT * FROM members WHERE email = :email"; | ||
|
|
||
| private static final RowMapper<Member> MEMBER_ROW_MAPPER = (rs, rowNum) -> parseResultSetToMember(rs); |
|
|
||
| Optional<Member> getMemberByEmail(String email); | ||
|
|
||
| boolean deleteMemberById(UUID id); |
There was a problem hiding this comment.
again, we shouldn't be introducing methods that are solely used in tests (unless i missed something where it was used elsewhere)
|
backend tests and sonarcloud failing |




SQL files are better tested via integration testing. MemberSqlRepoTest is now being tested via SpringBootTest rather than being a unit test.