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.

Sign up for GitHub

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

Jump to bottom

Generate separate coverage reports for frontend and backend #7151

Merged
merged 17 commits into from
Jul 31, 2019
Merged

Generate separate coverage reports for frontend and backend #7151

merged 17 commits into from
Jul 31, 2019

Conversation

lilithxxx
Copy link
Contributor

@lilithxxx lilithxxx commented Jul 16, 2019 •
edited
Loading

Explanation

Generate separate coverage reports for frontend and backend. This PR also tries to compare coverage of the PR with the base(develop branch) and not the head.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The linter/Karma presubmit checks have passed.
    • These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
  • The PR is made from a branch that's not called "develop".
  • The PR has an appropriate "CHANGELOG: ..." label (If you are unsure of which label to add, ask the reviewers for guidance).
  • The PR follows the style guide.
  • The PR addresses the points mentioned in the codeowner checks for the files/folders changed. (See the codeowner's wiki page.)
  • The PR is assigned to an appropriate reviewer.
    • If you're a new contributor, please ask on Gitter for someone to assign a reviewer and don't tick this checkbox.
    • If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue. Do not only request the review but also add the reviewer as an assignee.

@lilithxxx lilithxxx requested a review from apb7 as a code owner July 16, 2019 21:28
Rishav Chakraborty added 2 commits July 17, 2019 03:21
apb7
apb7 approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@apb7 apb7 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, @lilithxxx. Now that #7152 is in, can you please remove the TODO assigned to you above, in the frontend tests section so that we can get this in?

@apb7 apb7 assigned lilithxxx and unassigned apb7 Jul 18, 2019
@oppiabot
Copy link

oppiabot bot commented Jul 18, 2019

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@lilithxxx
Copy link
Contributor Author

@apb7 Done

@lilithxxx lilithxxx assigned apb7 and unassigned lilithxxx Jul 18, 2019
apb7
apb7 previously requested changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@apb7 apb7 left a comment

Choose a reason for hiding this comment

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

Hi @lilithxxx, can you add a codecov badge for front-end tests coverage? Also, I think we're waiting for your response on the corresponding email thread (I don't think we can merge this PR until we resolve the issue mentioned by Sean in the email thread).

@apb7 apb7 assigned lilithxxx and unassigned apb7 Jul 19, 2019
@lilithxxx lilithxxx changed the title Remove codecov from backend tests Generate separate coverage reports for frontend and backend Jul 25, 2019
Rishav Chakraborty added 2 commits July 25, 2019 23:51
@codecov
Copy link

codecov bot commented Jul 25, 2019

Codecov Report

Merging #7151 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #7151   +/-   ##
========================================
  Coverage    89.33%   89.33%           
========================================
  Files         1198     1198           
  Lines        96869    96869           
  Branches      2471     2471           
========================================
  Hits         86534    86534           
  Misses        9823     9823           
  Partials       512      512
Flag Coverage Δ
#backend 98.63% <ø> (?)
#frontend 71.16% <ø> (?)

@codecov
Copy link

codecov bot commented Jul 25, 2019 •
edited
Loading

Codecov Report

Merging #7151 into develop will decrease coverage by 9.38%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #7151      +/-   ##
===========================================
- Coverage    98.81%   89.42%   -9.38%     
===========================================
  Files          381     1204     +823     
  Lines        64562    98345   +33783     
  Branches         0     2635    +2635     
===========================================
+ Hits         63791    87943   +24152     
- Misses         771     9768    +8997     
- Partials         0      634     +634
Flag Coverage Δ
#backend 98.81% <ø> (?)
#frontend 71.49% <ø> (?)
Impacted Files Coverage Δ
.../collection/SearchExplorationsBackendApiService.ts 100% <0%> (ø)
...es/dev/head/domain/skill/SkillObjectFactorySpec.ts 100% <0%> (ø)
.../domain/topic/SubtopicPageContentsObjectFactory.ts 100% <0%> (ø)
...ead/expressions/ExpressionSyntaxTreeServiceSpec.ts 100% <0%> (ø)
...s/dev/head/services/AssetsBackendApiServiceSpec.ts 99.12% <0%> (ø)
...yer-page/services/answer-classification.service.ts 86.67% <0%> (ø)
...editor-page/services/story-editor-state.service.ts 96% <0%> (ø)
...ctionInput/directives/FractionInputRulesService.ts 100% <0%> (ø)
...domain/skill/EditableSkillBackendApiServiceSpec.ts 100% <0%> (ø)
...v/head/domain/objects/FractionObjectFactorySpec.ts 100% <0%> (ø)
... and 813 more

@brianrodri
Copy link
Contributor

Since this was resolved in the thread, I think we can merge now. @lilithxxx, could you please add the badge to the README so we can merge?

@lilithxxx lilithxxx requested a review from DubeySandeep as a code owner July 26, 2019 15:19
@oppiabot
Copy link

oppiabot bot commented Jul 27, 2019

Hi @lilithxxx. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@lilithxxx
Copy link
Contributor Author

@apb7 what do you say?

Codecov does not support adding badges according to flags currently(i.e we cannot add a badge only for the front-end tests). See here. Should I add a badge for the entire project then?

@lilithxxx lilithxxx requested a review from kevinlee12 as a code owner July 28, 2019 18:05
@seanlip
Copy link
Member

seanlip commented Jul 28, 2019

Just a note -- @kevinlee12 is now leading the dev workflow team, taking over from @apb7 . Adding him here and assigning him for a response.

Thanks!

@kevinlee12
Copy link
Contributor

@lilithxxx, does the entire coverage accumulate both frontend and backend?

@lilithxxx
Copy link
Contributor Author

@kevinlee12 do you mean the coverage on codecov website? If so, then yes.

This PR actually will enforce two separate checks and two separate coverage reports

@lilithxxx lilithxxx removed their assignment Jul 29, 2019
@kevinlee12
Copy link
Contributor

Ah, my point was to your question for Apurv about adding a badge for the entire project. Are you planning to add one badge or two badges?

@lilithxxx
Copy link
Contributor Author

Apurv said to add a badge for the front-end and we can't do that. We can only add a badge for the entire coverage. Should I do that?

@kevinlee12
Copy link
Contributor

If it’s for the entire codebase, let’s go ahead with that.

@lilithxxx
Copy link
Contributor Author

@kevinlee12 Done!

kevinlee12
kevinlee12 approved these changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@kevinlee12 kevinlee12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, thanks @lilithxxx!

@kevinlee12
Copy link
Contributor

@DubeySandeep, code owner review please!

@kevinlee12 kevinlee12 dismissed apb7’s stale review July 31, 2019 01:08

Dismissing Apurv's review since I'm taking over.

@kevinlee12 kevinlee12 merged commit 5335d8b into oppia:develop Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DubeySandeep DubeySandeep DubeySandeep approved these changes

@kevinlee12 kevinlee12 kevinlee12 approved these changes

@apb7 apb7 apb7 left review comments

Assignees

@lilithxxx lilithxxx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lilithxxx @brianrodri @seanlip @kevinlee12 @DubeySandeep @apb7