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

Mocked credentials for testing purposes #152

Conversation

jpoo90
Copy link
Contributor

@jpoo90 jpoo90 commented Apr 24, 2017

Closes #149

  • Added a mock for the credentials module.
  • Removed hardcoded credentials from bootstrap.spec.jsx

@thescientist13
Copy link
Owner

@jpoo90
There is a mock credentials class in test/mocks, would that be sufficient rather than making the new file?
https://github.com/thescientist13/github-dashboard/blob/master/test/mocks/user-credentials.json

@jpoo90
Copy link
Contributor Author

jpoo90 commented Apr 24, 2017

@thescientist13 I don't think so. In the bootstrap.tsx we need to mock the module itself. https://github.com/thescientist13/github-dashboard/blob/master/src/components/bootstrap/bootstrap.tsx#L29

test/mocks has a json with the credentials. In that case, we would need to mock the module, import the user-credentials.json and then return the json from the module mock.

Maybe I'm missing something from the mocking structure you have in place. But I think it's easier to mock the module itself and re-use when necessary.

@thescientist13
Copy link
Owner

@jpoo90
I see what you mean.

So could we move credentials.ts to test/mocks? Maybe user-credentials.json should be deprecated then? (depending on its usage in the app)

@jpoo90
Copy link
Contributor Author

jpoo90 commented Apr 24, 2017

@thescientist13

We can't move the mock to test/mocks. There as an open discussion in Jest's repo about it. I agree that we can depreacte user-credentials.json and use the mock module instead.

If you are OK with having the __mock__ folder, I can replace use it to replace user-credentials.json

@thescientist13
Copy link
Owner

We can't move the mock to test/mocks. There as an open discussion in Jest's repo about it.

Well, I guess that settles that then.

If you are OK with having the mock folder, I can replace use it to replace user-credentials.json

Sounds like a plan! 👍

@thescientist13
Copy link
Owner

ok to test

@thescientist13
Copy link
Owner

thescientist13 commented Apr 24, 2017

Looks like Cobertura is unhappy for the new code without coverage credentials.ts? I think adding it may have caused the overall coverage to do down, unless it can be ignored somehow?
http://www.thegreenhouse.io:8080/job/CI-github-dashboard/145/console

18:43:35 Cobertura coverage report found.
18:43:35 Code coverage enforcement failed for the following metrics:
18:43:35     Methods's stability is 83.33 and set mininum stability is 87.5.
18:43:35     Lines's stability is 89.49 and set mininum stability is 93.09.
18:43:35 Setting Build to unstable.

@thescientist13
Copy link
Owner

SOOOOOOOO close

19:46:08 Code coverage enforcement failed for the following metrics:
19:46:08     Methods's stability is 86.46 and set mininum stability is 87.5.
19:46:08     Lines's stability is 92.49 and set mininum stability is 93.09.

@jpoo90
Copy link
Contributor Author

jpoo90 commented Apr 24, 2017

@thescientist13

I can't find why the coverage dropped. This is the difference between master and this branch.

Master:
master

PR:
pullrequest

Do you have any recommendations ? I don't see how the updated mock affected the interface coverage 😒

@thescientist13
Copy link
Owner

ok to test

@thescientist13
Copy link
Owner

Can one of the admins verify this patch?

@thescientist13
Copy link
Owner

ok to test

@thescientist13
Copy link
Owner

Can one of the admins verify this patch?

@thescientist13
Copy link
Owner

Can one of the admins verify this patch?

@thescientist13
Copy link
Owner

@jpoo90
i fine tuned the metric targets for Cobertura to set thresholds instead of auto update. Not sure why but looks good to me.

fyi: merge conflict

@jpoo90 jpoo90 force-pushed the bug/issue-149-unhandled-error-when-testing branch from 508a592 to de58210 Compare April 25, 2017 01:21
@jpoo90
Copy link
Contributor Author

jpoo90 commented Apr 25, 2017

@thescientist13 Solved merge conflicts

Copy link
Owner

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

👍

@thescientist13 thescientist13 merged commit 674a41a into thescientist13:master Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants