-
Notifications
You must be signed in to change notification settings - Fork 74
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
Codecov & Botonic workflows #PLA-226 #1177
Conversation
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.
Nice!! 👏👏👏
push: | ||
paths: | ||
- 'packages/botonic-cli/**' | ||
- '.github/workflows/botonic-cli-tests.yml' |
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.
are you sure that you need to include the workflow file itself?
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.
From what I understood from the docs, a git diff is performed so I think it is needed.
I think I did a test on that but I will double check
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 did some checks on https://github.com/hubtype/botonic/tree/chore/codecov-temp branch and I confirm that line is needed: if we remove it, changes on the .yml file won't trigger the workflow.
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.
👏
run: scripts/qa/lint-d-ts.sh packages/botonic-core | ||
run: scripts/qa/lint-d-ts.sh ./packages/$PACKAGE | ||
|
||
- name: Upload coverage to codecov |
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.
can you try this example https://stackoverflow.com/a/59798779/145289 to see if you can have the codecov config in a single file?
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.
From what I have read (actions/runner#646), that would the case for reusing a pure bash/docker/JS action, not to call one action from another.
I didn't tried because of that link but I can give it a try.
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.
Nice!! 💯 go for it after taking a look at @dpinol suggestions!
@@ -4,10 +4,9 @@ on: | |||
push: | |||
paths: | |||
- '*' |
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, can you add '' and 'packages/' for all packages?
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.
included in 7e31727
|
||
jobs: | ||
botonic-plugin-nlu-tests: | ||
name: Botonic nlu tests | ||
runs-on: ubuntu-latest | ||
env: | ||
PACKAGE: botonic-nlu | ||
steps: | ||
- name: Checking out to current branch (Step 1 of 7) | ||
uses: actions/checkout@v1 |
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.
make the most of this PR to try to upgrade to actions/checkout@v2, actions/cache@v2 and actions/setup-node@v2
luckily you can do a replace-all for this
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.
updated in 6076e6a
Codecov Report
@@ Coverage Diff @@
## master #1177 +/- ##
===========================================
+ Coverage 54.38% 64.57% +10.19%
===========================================
Files 73 202 +129
Lines 2133 5251 +3118
Branches 652 990 +338
===========================================
+ Hits 1160 3391 +2231
- Misses 953 1147 +194
- Partials 20 713 +693
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
👏
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 would like to see another PR that adds the codecov upload for all packages so we make explicit that we have these packages untested (and we can see progress in the future).
This, I guess, will require to add a npm run test
to them, despite this will run nothing.
But confirm with @dpinol and botonic team
Also I see lot of repetition. maybe we can think in a refactor to put a "generic workflow" that is called with params for each workflow
@@ -1,6 +1,12 @@ | |||
name: Botonic cli tests | |||
|
|||
on: [push] | |||
on: |
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.
You are not uploading the coverage for this package.
I see it isn't running any test (there isn't any npm run test
).
I would prefer to also upload this coverage that will be 0%, but this will make visible that we don't have any testing for this package.
But this can go in a separate PR to get this merged
Description
Context
Codecov
Configuring codecov will allow us to improve the coverage on every push (or at least keep it as the level of previous push)
The configuration in codecov.yml includes an initial setup with the following:
You can see the reports in https://codecov.io/gh/hubtype/botonic
Trigger Github workflows only when necessary
Prior to this PR, all workflows were triggered, no matter where the change was coming from.
Now, each workflow is triggered only when the change is originated in the path where the workflow operates. For instance, changes in packages/botonic-plugin-dynamodb/** will trigger botonic-plugin-dynamo-tests.yml workflow only. Of course, if we change a workflow file, it will be executed once it is pushed.
Approach taken / Explain the design
To document / Usage example
You can see last generated reports at https://codecov.io/gh/hubtype/botonic
Testing
I executed some tests to see that results from the different paths appear in the codecov page.
Regarding the event that triggers the workflows, I also did some tests but, as I have changed all workflows and paths sometimes are hard to validate, something might have fallen through the cracks, please let me know if you see that workflows are not being triggered upon your changes.