Add unit tests for issues and PRs#505
Conversation
SAN-MUYUN
left a comment
SAN-MUYUN
left a comment
There was a problem hiding this comment.
Overall the code written is neat and concise. Appreciate the effort to add more test cases for Issues and PRs. 👍
There will be some merge conflicts or overwritten of code when PR #499(Refactor: split issue and pull request logic for improved clarity) is merged.
It would be good to separate testing for Issues and PRs for all the test cases. There are also some repetition in the testcases which you might want to re-consider.
| } | ||
|
|
||
| public static createPhaseBugReportingIssue(githubIssue: GithubIssue): Issue { | ||
| public static createFromGithubIssue(githubIssue: GithubIssue): Issue { |
There was a problem hiding this comment.
Appreciate the effort to rename this method and make it more independent of CATcher. Take note that this may result in merge conflict with PR #499 .
| const issueUpdatedEarlier: Issue = Issue.createPhaseBugReportingIssue(ISSUE_UPDATED_EARLIER); | ||
| const issueUpdatedLater: Issue = Issue.createPhaseBugReportingIssue(ISSUE_UPDATED_LATER); | ||
| const issueUpdatedEarlier: Issue = Issue.createFromGithubIssue(ISSUE_UPDATED_EARLIER); | ||
| const issueUpdatedLater: Issue = Issue.createFromGithubIssue(ISSUE_UPDATED_LATER); |
There was a problem hiding this comment.
Similar to above, the naming might cause merge conflicts with #499 .
| expect(pr.assignees).toEqual([USER_ANUBHAV.loginId]); | ||
| }); | ||
|
|
||
| it('should handle closed pull requests correctly', () => { |
There was a problem hiding this comment.
For this test case, the testing seems fine to me, but the description can be more precise. Instead of saying "handle" closed PR correctly, which could be vague, it might be better to rephrase it as "creates" closed pull requests correctly
| expect(pr.reviewDecision).toEqual(ReviewDecision.APPROVED); | ||
| }); | ||
|
|
||
| it('should handle draft pull requests correctly', () => { |
|
|
||
| expect(pr.issueOrPr).toEqual('PullRequest'); | ||
| expect(draftPr.issueOrPr).toEqual('PullRequest'); | ||
| expect(issue.issueOrPr).toEqual('Issue'); |
There was a problem hiding this comment.
Good attempt to try testing if issues and PRs are created correctly. However, this test case seems a little repetitive with the previous ones since they both creates an Issue object and check for issueOrPr property of this object. It might be better to move this test case under the test file for filters, to check if pull requests can be correctly identified among several PRs and issues
| const issue = Issue.createFromGithubIssue(ISSUE_WITH_EMPTY_DESCRIPTION); | ||
|
|
||
| expect(pr.headRepository).toEqual('testuser/WATcher-test'); | ||
| expect(issue.headRepository).toBeUndefined(); |
There was a problem hiding this comment.
It would be good if you can test issues and PRs separately, rather than mixing issues and PRs together. It might be fine to leave it for now, but it will need to be separated after #499 is merged.
2 participants
Summary:
Fixes #453
Add test case coverage for issues and PRs, refactoring CATcher code into WATcher context during the process
Type of change:
Changes Made:
githubpullrequest.constants.tswith mock PR data for testingProposed Commit Message:
Checklist: