Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Add coverage report #131

Merged
merged 6 commits into from
Mar 11, 2020
Merged

Add coverage report #131

merged 6 commits into from
Mar 11, 2020

Conversation

hanzei
Copy link
Contributor

@hanzei hanzei commented Mar 5, 2020

Summary

Ticket Link

@hanzei hanzei added the Work In Progress Not yet ready for review label Mar 5, 2020
@hanzei hanzei requested a review from mgdelacroix March 6, 2020 10:14
@hanzei hanzei added 2: Dev Review Requires review by a core committer and removed Work In Progress Not yet ready for review labels Mar 6, 2020
.codecov.yml Outdated Show resolved Hide resolved
.codecov.yml Outdated Show resolved Hide resolved
@@ -85,6 +85,11 @@ test-all:
@echo Running all tests
$(GO) test -mod=vendor -race -v -tags 'unit e2e' $(GO_PACKAGES)

.PHONY: coverage
coverage:
$(GO) test -mod=vendor -race -tags unit -coverprofile=coverage.txt ./...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to only use unit test coverage as it should be the same as the e2e one

Copy link
Member

Choose a reason for hiding this comment

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

I would add both tags to the coverage, as there are things that we are only testing in e2e (the client creation and connection for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with also running the e2e test is that they (have to) run in docker. Which makes it a bit hard to extract the coverage report. Would you be fine just running the unit tests for now and I open a ticket for the e2e tests?

Copy link
Member

Choose a reason for hiding this comment

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

I think that is a good idea, opening another ticket to refine it later.

@hanzei hanzei requested a review from jespino March 6, 2020 10:21
@hanzei
Copy link
Contributor Author

hanzei commented Mar 6, 2020

You can see the report at https://codecov.io/gh/mattermost/mmctl/branch/mmctl

.circleci/config.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@@ -85,6 +85,11 @@ test-all:
@echo Running all tests
$(GO) test -mod=vendor -race -v -tags 'unit e2e' $(GO_PACKAGES)

.PHONY: coverage
coverage:
$(GO) test -mod=vendor -race -tags unit -coverprofile=coverage.txt ./...
Copy link
Member

Choose a reason for hiding this comment

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

I would add both tags to the coverage, as there are things that we are only testing in e2e (the client creation and connection for example)

@codecov-io
Copy link

codecov-io commented Mar 6, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@6415a09). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #131   +/-   ##
=========================================
  Coverage          ?   60.13%           
=========================================
  Files             ?       26           
  Lines             ?     1999           
  Branches          ?        0           
=========================================
  Hits              ?     1202           
  Misses            ?      760           
  Partials          ?       37           

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 6415a09...66c6bf7. Read the comment docs.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Some things to be addressed, mainly raised by @mgdelacroix, but otherwise looks good to me.

Co-Authored-By: Miguel de la Cruz <miguel@mcrx.me>
Copy link
Member

@mgdelacroix mgdelacroix left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this!!

@hanzei hanzei added the Do Not Merge Should not be merged until this label is removed label Mar 11, 2020
@hanzei
Copy link
Contributor Author

hanzei commented Mar 11, 2020

I forgot to add a badge. Will do later today.

@hanzei hanzei requested a review from mgdelacroix March 11, 2020 11:25
@hanzei hanzei removed the Do Not Merge Should not be merged until this label is removed label Mar 11, 2020
Copy link
Member

@mgdelacroix mgdelacroix left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mgdelacroix mgdelacroix added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 11, 2020
@mgdelacroix mgdelacroix merged commit a701ab4 into master Mar 11, 2020
@mgdelacroix mgdelacroix deleted the mmctl branch March 11, 2020 13:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants