Add mocks and unit tests to http and msgraph packages #2

cpurta · 2019-11-12T20:37:20Z

Added a mocks package that mocks out the functionality of some the
interfaces needed by the oauth handlers. This allows testing of each
error check within the oauth handler functions. I also added
github.com/jarcoal/httpmock as a dependecy to test both the oauth code
to token exchange and each of the calls that will be made out to the
microsoft graph API.

The tests cover how all errors are encountered for the oauth connection.
They also ensure that access token refreshing happens when the token is
expired. This tests to make sure the golang oauth client works out of
the box for this plugin.

This will also resolve #3

- added OAuth2 credentials a config values

Added a mocks package that mocks out the functionality of some the interfaces needed by the oauth handlers. This allows testing of each error check within the oauth handler functions. I also added github.com/jarcoal/httpmock as a dependecy to test both the oauth code to token exchange and each of the calls that will be made out to the microsoft graph API. The tests cover how all errors are encountered for the oauth connection. They also ensure that access token refreshing happens when the token is expired. This tests to make sure the golang oauth client works out of the box for this plugin.

levb

Overall 👍, just a couple nits, and suggested #3 as a separate PR.

server/http/http.go

server/http/oauth2_complete_test.go

levb · 2019-11-13T14:25:33Z

Also @cpurta please sign the CLA in the link above.

levb · 2019-11-13T15:02:06Z

@cpurta dev has been merged, so you can re-base/re-point to master now.

cpurta · 2019-11-13T16:53:17Z

Signed the CLA yesterday and now seeing the bot recognize that. Also just merge the master into my branch and everything is looking up to date.

This includes moving all of the tests to their own subpackages to allow for a cleaner workspace. Move all the mock interfaces to the test_http package as they are only needed there currently. Update mock interfaces to use github.com/stretchr/testify/mock to conform to Mattermost mock standards. Also addresses any naming convention comments in this commit as well.

@cpoile

Per @cpoile we are migrating the mocked interfaces from stretchr/testify/mock to instead be generated and used via gomock. I have added a new makefile target to generate the mock interfaces using mockgen. In addition I have generated those mocks to be used in the unit tests in the test_http package. All of those unit tests have been updated to use the generated mocks as well.

cpoile

Nice @cpurta, looks better now! I made a comment about the design and what the mocks are really testing. Once that's solved, this should be good to go.

server/http/test_http/oauth2_complete_test.go

levb

Generally, looks good, just a few nits. I'll wait until @cpoile is happy with the gomock changes, then will do one final pass?

server/http/test_http/mock_response_writer.go

server/http/test_http/oauth2_complete_test.go

cpoile · 2019-11-18T15:25:27Z

@cpurta Also, as soon as you want to let us know the PR is ready for another review, hit the refresh symbol next to our names in the top right -- that will ping us through the Mattermost Github plugin that it's time to take another look. :)

@levb

Refactored the mocks to be a part of the testcase struct and then pass those to a `setupMocks` function which allow for custom mock behavior. This sigificantly reduces the code for setup and allows for more customizable mocks. Also addressing some of @levb comments on package names for the test folders and naming convention.

cpoile

Awesome, looking better, almost there. :)
Let me know if you have any questions (I'm always on MM).

server/http/testhttp/oauth2_complete_test.go

server/http/testhttp/oauth2_connect_test.go

cpoile

Thanks @cpurta!
Just a couple more cases of not needed test cases fields.

server/http/testhttp/oauth2_complete_test.go

server/http/testhttp/oauth2_connect_test.go

cpoile

Awesome, thanks @cpurta!

cpoile · 2019-11-21T16:58:59Z

@levb Final pass?

levb

LGTM!

…a single user" by added additional logging (#245) * [MI-2664]:Fixed issue #244 on ms calender to add additional logging (#2) * [MI-2664]:Fixed issue #244 on ms calender to add additional logging * [MI-2664]:Added comment for skipping user * [MI-2664]:Fixed unit test cases * [MI-2664]:Fix lint errors * [MI-2664]:Added conditional logging * [MI-2664]:Added constants for limit * [MI-2664]:Fixed review comments * [MI-2664]:Fixed review comments * [MI-2687]:Fixed review comments given by mattermost team on issue#244 of mscalender plugin (#3) * [MI-2687]:Fixed review comments given by mattermost team on issue #244 of mscalender plugin * [MI-2687]:Fixed unit test cases * [MI-2687]:Fixed review fixes * [MI-2705]:Added summary for status sync job (#4) * [MI-2705]:Added summary for status sync job * [MI-2705]:Fixed review comments * Fixed lint errors (#243) * [MI-2705]:Fixed review comments * [MI-2705]:Fixed review comments --------- Co-authored-by: Manoj Malik <[email protected]> * [MM-244]:Fixed lint errors * [MM-244]:Added a check for summary to be nill * [MM-244]:Added addtional check for user error * [MM-244]:Fixed lint errors * [MM-244]:Fixed review comments --------- Co-authored-by: Manoj Malik <[email protected]>

Lev Brouk and others added 6 commits October 31, 2019 06:24

command + http framework

a47b74c

Got a 404 on the hardcoded Mattermost Org - no Outlook

b3a9a25

/msoffice viewcal works on a personal account

8529c6a

- added OAuth2 credentials a config values

PR feedback

9cd3e7a

Update http.go and plugin.go with upstream/dev branch

a00e44b

levb mentioned this pull request Nov 13, 2019

MM-18122 mattermost/mattermost-plugin-jira#363

Closed

levb suggested changes Nov 13, 2019

View reviewed changes

levb requested a review from cpoile November 13, 2019 14:24

levb added the 2: Dev Review Requires review by a core committer label Nov 13, 2019

levb added this to the 0.1.0 milestone Nov 13, 2019

cpurta changed the base branch from dev to master November 13, 2019 16:44

Fix merge conflicts from upstream/master branch

5cb1074

cpurta mentioned this pull request Nov 13, 2019

Use github.com/stretchr/testify/mock for unit tests #3

Closed

hanzei added Do Not Merge Should not be merged until this label is removed and removed Do Not Merge Should not be merged until this label is removed labels Nov 13, 2019

Chris Purta added 2 commits November 14, 2019 13:05

Use getUserRequest for oauth connect tests

2f2e12b

hanzei requested a review from levb November 15, 2019 04:49

cpoile requested changes Nov 15, 2019

View reviewed changes

server/http/test_http/oauth2_complete_test.go Outdated Show resolved Hide resolved

Refactor how mocks are built for tests

d5a13b4

levb added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Nov 18, 2019

levb reviewed Nov 18, 2019

View reviewed changes

server/http/test_http/mock_response_writer.go Outdated Show resolved Hide resolved

server/http/test_http/oauth2_complete_test.go Outdated Show resolved Hide resolved

server/http/test_http/oauth2_complete_test.go Outdated Show resolved Hide resolved

cpoile requested changes Nov 18, 2019

View reviewed changes

server/http/test_http/oauth2_complete_test.go Outdated Show resolved Hide resolved

server/http/test_http/oauth2_complete_test.go Outdated Show resolved Hide resolved

cpurta requested review from levb and cpoile November 19, 2019 18:04

cpoile requested changes Nov 20, 2019

View reviewed changes

server/http/testhttp/oauth2_complete_test.go Outdated Show resolved Hide resolved

server/http/testhttp/oauth2_connect_test.go Outdated Show resolved Hide resolved

Create mocks in test run function and pass to tc struct after setup

0633721

cpurta requested a review from cpoile November 20, 2019 17:29

cpoile requested changes Nov 21, 2019

View reviewed changes

Move redundant test case fields to testing block

5d62a39

cpurta requested a review from cpoile November 21, 2019 16:51

cpoile approved these changes Nov 21, 2019

View reviewed changes

levb approved these changes Nov 21, 2019

View reviewed changes

levb added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Nov 21, 2019

levb merged commit 5bd5bd2 into mattermost:master Nov 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add mocks and unit tests to http and msgraph packages #2

Add mocks and unit tests to http and msgraph packages #2

cpurta commented Nov 12, 2019 •

edited

Loading

levb left a comment

levb commented Nov 13, 2019

levb commented Nov 13, 2019

cpurta commented Nov 13, 2019

cpoile left a comment

levb left a comment

cpoile commented Nov 18, 2019

cpoile left a comment

cpoile left a comment

cpoile left a comment

cpoile commented Nov 21, 2019

levb left a comment

Add mocks and unit tests to http and msgraph packages #2

Add mocks and unit tests to http and msgraph packages #2

Conversation

cpurta commented Nov 12, 2019 • edited Loading

levb left a comment

Choose a reason for hiding this comment

levb commented Nov 13, 2019

levb commented Nov 13, 2019

cpurta commented Nov 13, 2019

cpoile left a comment

Choose a reason for hiding this comment

levb left a comment

Choose a reason for hiding this comment

cpoile commented Nov 18, 2019

cpoile left a comment

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

cpoile left a comment

Choose a reason for hiding this comment

cpoile commented Nov 21, 2019

levb left a comment

Choose a reason for hiding this comment

cpurta commented Nov 12, 2019 •

edited

Loading