Feat: add any-of-labels option #319
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| import {Issue} from '../src/classes/issue'; | ||
| import {IssuesProcessor} from '../src/classes/issues-processor'; | ||
| import {IIssuesProcessorOptions} from '../src/interfaces/issues-processor-options'; | ||
| import {DefaultProcessorOptions} from './constants/default-processor-options'; | ||
| import {generateIssue} from './functions/generate-issue'; | ||
|
|
||
| describe('any-of-labels option', () => { | ||
| test('should do nothing when not set', async () => { | ||
| const sut = new IssuesProcessorBuilder() | ||
| .emptyAnyOfLabels() | ||
| .issues([{labels: [{name: 'some-label'}]}]) | ||
| .build(); | ||
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(1); | ||
|
There was a problem hiding this comment. I saw that the other tests are not using this method to check the length yet it's the most appropriate one ❤️ There was a problem hiding this comment. Yeah I'm still not 100% satisfied with that one cause there is some implementation detail leaking out. It would probably be worth it to explore adding a custom matcher for this in the future, maybe? Like |
||
| }); | ||
|
|
||
| test('should skip it when none of the issue labels match', async () => { | ||
| const sut = new IssuesProcessorBuilder() | ||
| .anyOfLabels('skip-this-issue,and-this-one') | ||
| .issues([{labels: [{name: 'some-label'}, {name: 'some-other-label'}]}]) | ||
| .build(); | ||
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('should skip it when the issue has no labels', async () => { | ||
| const sut = new IssuesProcessorBuilder() | ||
| .anyOfLabels('skip-this-issue,and-this-one') | ||
| .issues([{labels: []}]) | ||
| .build(); | ||
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(0); | ||
| }); | ||
|
|
||
| test('should process it when one of the issue labels match', async () => { | ||
| const sut = new IssuesProcessorBuilder() | ||
| .anyOfLabels('skip-this-issue,and-this-one') | ||
| .issues([{labels: [{name: 'some-label'}, {name: 'skip-this-issue'}]}]) | ||
| .build(); | ||
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(1); | ||
| }); | ||
|
|
||
| test('should process it when all the issue labels match', async () => { | ||
| const sut = new IssuesProcessorBuilder() | ||
| .anyOfLabels('skip-this-issue,and-this-one') | ||
| .issues([{labels: [{name: 'and-this-one'}, {name: 'skip-this-issue'}]}]) | ||
| .build(); | ||
|
|
||
| await sut.processIssues(); | ||
|
|
||
| expect(sut.staleIssues).toHaveLength(1); | ||
| }); | ||
| }); | ||
|
|
||
| class IssuesProcessorBuilder { | ||
|
There was a problem hiding this comment. I prefer to have all the "tests arrange" in the top of the file. There was a problem hiding this comment. Well, ideally that fixture would be moved into a separate file and enriched to be shared and reused by all tests. But even if it stays here, I don't really see much value having it on top in this case because 1) the test arrangement would be the instantiation of the builder, not its declaration, 2) the builder itself, albeit useful, is not that interesting, and anyone going through the test suite should be able to get what it does just by looking at how it's used tbh, and 3) having the |
||
| private _options: IIssuesProcessorOptions; | ||
| private _issues: Issue[]; | ||
|
|
||
| constructor() { | ||
| this._options = {...DefaultProcessorOptions}; | ||
|
There was a problem hiding this comment. Why not assign the properties on creation to reduce the class size instead of using a constructor? There was a problem hiding this comment. Ah, a OO purist would say the constructor is the place to do it! lol. But really, I'm in the habit of doing it like this for more practical reasons: 1) if you add some more fields, and some of them are initialized via a constructor arg, and others are initialized inline where they are declared, that could get a bit disorganized, 2) it gives a good indication for potential refactorings, e.g. those default options could be injected, instead of imported (imagine we have some other predefined defaults, for example). |
||
| this._issues = []; | ||
| } | ||
|
|
||
| anyOfLabels(labels: string): IssuesProcessorBuilder { | ||
| this._options.anyOfLabels = labels; | ||
| return this; | ||
| } | ||
|
|
||
| emptyAnyOfLabels(): IssuesProcessorBuilder { | ||
| return this.anyOfLabels(''); | ||
| } | ||
|
|
||
| issues(issues: Partial<Issue>[]): IssuesProcessorBuilder { | ||
| this._issues = issues.map( | ||
| (issue, index): Issue => | ||
| generateIssue( | ||
| this._options, | ||
| index, | ||
| issue.title || 'Issue title', | ||
| issue.updated_at || '2000-01-01T00:00:00Z', // we only care about stale/expired issues here | ||
| issue.created_at || '2000-01-01T00:00:00Z', | ||
| issue.isPullRequest || false, | ||
| issue.labels ? issue.labels.map(label => label.name) : [] | ||
| ) | ||
| ); | ||
| return this; | ||
| } | ||
|
|
||
| build(): IssuesProcessor { | ||
| return new IssuesProcessor( | ||
| this._options, | ||
| async () => 'abot', | ||
| async p => (p === 1 ? this._issues : []), | ||
| async () => [], | ||
| async () => new Date().toDateString() | ||
| ); | ||
| } | ||
| } | ||
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 like the fact that you decided to put them in a dedicated file