-
Notifications
You must be signed in to change notification settings - Fork 0
[Feature/PE1-4] Add test coverage action file #3
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
base: develop
Are you sure you want to change the base?
Conversation
|
Scope of work: |
jimmy-lan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together! Just some minor problems that require your attention. See comments.
.github/workflows/coverage-test.yml
Outdated
| @@ -0,0 +1,43 @@ | |||
| name: Coverage-test | |||
|
|
|||
| # Controls when the action will run. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are explaining how GitHub workflow works. Since we are not using this as a tutorial, can you remove these comments?
.github/workflows/coverage-test.yml
Outdated
| # This workflow contains a single job called "build" | ||
| build: | ||
| # The type of runner that the job will run on | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using ubuntu-18.04 to be consistent with other workflows?
.github/workflows/coverage-test.yml
Outdated
|
|
||
| # Steps represent a sequence of tasks that will be executed as part of the job | ||
| steps: | ||
| # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should put more thoughts into this comment - try to remove "your".
.github/workflows/coverage-test.yml
Outdated
| @@ -0,0 +1,43 @@ | |||
| name: Coverage-test | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to name this file test-coverage.yml, instead of coverage-test. We also have a convention for workflows, so please name this something like CI - Test Coverage if possible.
.github/workflows/coverage-test.yml
Outdated
| branches: [ master ] | ||
|
|
||
| # Allows you to run this workflow manually from the Actions tab | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a good convention would be to set workflow_dispatch to {}. Otherwise, this looks incomplete (despite it is completed). Do you mind looking this up?
|
|
||
| # A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
| jobs: | ||
| # This workflow contains a single job called "build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we still have plenty of comments explaining how GitHub action works. Can you remove them?
In this pull request, a action file for test coverage is added.