-
Notifications
You must be signed in to change notification settings - Fork 24
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
Refactor test cases for Login Component, Session Model and Conflict Model #241
Refactor test cases for Login Component, Session Model and Conflict Model #241
Conversation
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.
Thanks for the PR! Just a few minor changes to make things clearer.
Actually this PR could have been broken down further into 2 PRs: one for the Login component and one for the Session model. In fact I'll probably encourage you to break down the second part of your test refactoring PR to 4 separate PRs: one for each spec you're refactoring - makes our job as reviewers easier (since we can review 1 spec at a time without having to review all 4) and also means that reviews will be faster. |
ok got it. I will break down the second part later. Thanks for the review, will do the changes soon. |
6a1c4d4
to
0aefb61
Compare
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.
Thanks for making the changes! Still a few that you might have missed out.
expect(component.startLoginProcess).toHaveBeenCalledWith(false); | ||
}); | ||
|
||
it('should call startLoginProcess with true in startIncludePrivateLoginProcess', () => { |
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.
Same comment as the one I made in line 39 above.
.pipe(assertSessionDataIntegrity()) | ||
.subscribe({ | ||
next: () => fail(), | ||
error: (err) => expect(err).toEqual(new Error(OPENED_PHASE_REPO_UNDEFINED)) | ||
}); | ||
of({ ...BUG_REPORTING_PHASE_SESSION_DATA, phaseBugReporting: null }) | ||
of({ ...VALID_SESSION_DATA, sessionRepo: [{ phase: Phase.issuesViewer, repo: null }] }) |
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.
Same comment as the one I made for line 54 above, also applies to lines 66 and 72.
tests/constants/session.constants.ts
Outdated
export const MULTIPLE_OPEN_PHASES_SESSION_DATA: SessionData = { | ||
...BUG_REPORTING_PHASE_SESSION_DATA, | ||
openPhases: [Phase.phaseBugReporting, Phase.phaseTeamResponse, Phase.phaseTesterResponse, Phase.phaseModeration] | ||
export const SESSION_DATA_WITH_INVALID_PHASE: SessionData = { |
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.
Is this constant being used in any of the tests?
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.
No, will remove the constant.
0aefb61
to
71a1b5c
Compare
3d344a7
to
02037c5
Compare
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.
LGTM, thanks!
Summary:
Fixes part of #85
Type of change:
Changes Made:
Proposed Commit Message: