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.

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

fix: Fixed tests running more than once and minor improvements #1095

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

devkapilbansal
Copy link
Member

Description

In github actions, checks were running multiple time unnecessarily.

Fixes #1093

Type of Change:

  • Quality Assurance

How Has This Been Tested?

Run in on my fork to ensure tests run only once and coverage report is generated too.

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #1095 (3aa3c87) into develop (8977489) will decrease coverage by 0.24%.
The diff coverage is n/a.

❗ Current head 3aa3c87 differs from pull request most recent head 7ee7793. Consider uploading reports for the commit 7ee7793 to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1095      +/-   ##
===========================================
- Coverage    94.99%   94.75%   -0.25%     
===========================================
  Files           38       37       -1     
  Lines         2058     2000      -58     
===========================================
- Hits          1955     1895      -60     
- Misses         103      105       +2     
Impacted Files Coverage Δ
app/api/email_utils.py 91.22% <0.00%> (-5.27%) ⬇️
app/database/db_types/JsonCustomType.py 73.33% <0.00%> (-3.14%) ⬇️
app/api/resources/mentorship_relation.py 95.03% <0.00%> (-2.05%) ⬇️
app/api/resources/user.py 89.70% <0.00%> (-0.67%) ⬇️
app/api/resources/admin.py 89.83% <0.00%> (-0.50%) ⬇️
app/database/models/task_comment.py 95.00% <0.00%> (-0.46%) ⬇️
run.py 96.00% <0.00%> (-0.16%) ⬇️
app/api/dao/admin.py 95.91% <0.00%> (-0.09%) ⬇️
app/api/dao/user.py 100.00% <0.00%> (ø)
app/api/jwt_extension.py 100.00% <0.00%> (ø)
... and 4 more

@@ -26,30 +26,25 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
python: [3.7, 3.9]
python: [3.7, 3.8, 3.9]
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is required please check on comment on vaishnavi's pr

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because sometimes code blocks failed for particular software/library version. We should test each version that we provide support for.

- name: Upload coverage to Codecov
uses: codecov/codecov-action@v1
with:
file: ./coverage.xml
name: codecov-umbrella
Copy link
Member

@epicadk epicadk May 9, 2021

Choose a reason for hiding this comment

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

Probably not related to this issue but upload coverage only if the maxtrix version is 3.7. Can you make this change too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want to upload coverage only when python version is 3.7?
If yes, then that can be done with a if block but why we need that?

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to upload coverage only when python version is 3.7?
If yes, then that can be done with a if block but why we need that?

Because it's uploading the coverage for both the versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should maintain coverage for all versions. May be in future we have some compatibility shims then coverage will change for versions

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how the python version is going to affect the coverage : /. Plus When the bot comments there is no way to differentiate between which coverage corresponds to which version.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was happening because all coverages have same name. Btw, I add a condition to upload coverage for Python 3.7

@vj-codes
Copy link
Member

@devkapilbansal any updates here? ^

@devkapilbansal devkapilbansal added the Status: Needs Review PR needs an additional review or a maintainer's review. label Jun 9, 2021
@devkapilbansal devkapilbansal requested review from a team, aditmehta9 and gaurivn and removed request for a team July 15, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Tests run more than once in Gtihub Actions
3 participants