Skip to content
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

Improve Github dsl test #214

Merged
merged 2 commits into from
Apr 12, 2017
Merged

Conversation

alex3165
Copy link
Contributor

@alex3165 alex3165 commented Apr 12, 2017

This PR improve the way GitHub.ts is tested by mocking GithubAPI class so that it is using mock data instead of doing call to API.

What do you think about it ? If the approach seems good for you I can write the missing tests, it should be straightforward now to add any test on top of this.

@DangerCI
Copy link

DangerCI commented Apr 12, 2017

Warnings
⚠️ Please add a changelog entry for your changes.

Generated by 🚫 dangerJS

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #214 into master will increase coverage by 0.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   66.21%   66.75%   +0.53%     
==========================================
  Files          33       33              
  Lines         743      743              
  Branches      102      102              
==========================================
+ Hits          492      496       +4     
+ Misses        251      247       -4
Impacted Files Coverage Δ
source/platforms/GitHub.ts 46.51% <ø> (+9.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c7000...5d3fb95. Read the comment docs.

Copy link
Member

@orta orta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this looks like a much nicer pattern. Thanks @alex3165 👍

const fixtures = resolve(__dirname, "fixtures")

/** Returns JSON from the fixtured dir */
export const requestWithFixturedJSON = async (path: string): Promise<() => Promise<any>> => () => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, this type annotation at the end! :D

@orta orta merged commit 9baa8a5 into danger:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants