AXON-113: First test for server pull requests #110
Conversation
bwieger-atlassian-com
requested review from
sdzh-atlassian,
cabella-dot and
marcomura
as code owners
| }); | ||
| }); | ||
|
|
||
| const getTaskCountDataV8 = { |
There was a problem hiding this comment.
Test data. Could be placed in a supporting file.
There was a problem hiding this comment.
For something this small I'd say it might be better to keep it here, but no strong opinion :)
There was a problem hiding this comment.
also, SUPER NITPICK (sorry): getTaskCountDataV8 and getPullRequestData sound like function names to me, so I was actually a bit confused when reading the usage above :P
Again, this is like new level of pettiness as far as nitpicks go, feel free to ignore 😁
There was a problem hiding this comment.
I agree, I think it would be better if they were named like constants! i.e. TASK_COUNT_DATA_V8
| headers: {}, | ||
| }; | ||
|
|
||
| const getPullRequestData = { |
There was a problem hiding this comment.
Do we actually need the realistic PR response here? I'd normally suggest that we minimize stuff like this and only keep the necessary fields for tests - but I'm not sure if it might be needed for other tests or something
There was a problem hiding this comment.
Honestly, not familiar enough with the code to know what is needed vs not.
For now, its super easy to enter debug mode and copy-paste a real example. So that's what I went with.
There was a problem hiding this comment.
Maybe we can create a mock values file to hold all of these to keep the tests readable
There was a problem hiding this comment.
^^ only if we plan on adding more cases to this test
| if ( | ||
| url.includes('/rest/api/1.0/projects/owner/repos/repo/pull-requests/PR-1/blocker-comments?count=true') | ||
| ) { | ||
| return Promise.resolve(getTaskCountDataV8); |
There was a problem hiding this comment.
NIT: (and I might be wrong) any reason not to just do return getTaskCountDataV8, return {data: getPullRequestData } and throw new Error('Not Found') here? Basically, do we actually need the Promise.* if the closure is already async?
A first pass at make a test for this file.
Let me know if you like the structure. Then I'll go do some more.