Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

feat: Github action integrated with codecov #632

Merged
merged 8 commits into from
Aug 22, 2020

Conversation

satya7289
Copy link
Contributor

@satya7289 satya7289 commented Jul 29, 2020

Description

Integrate codecov to the portal, so that we are able to check the code coverage of the portal. For this, I choose Github action(as suggested by @SanketDG) and also use github action to CI/CD and remove Travis ci.

Fixes #631

Type of Change:

Delete irrelevant options.

  • Code
  • Quality Assurance
  • Documentation

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

We are able to see the codecov report's on the PR and also we can see the checks of github.

Checklist:

Delete irrelevant options.

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

  • My changes generate no new warnings

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #632   +/-   ##
==========================================
  Coverage           ?   89.49%           
==========================================
  Files              ?       49           
  Lines              ?     2693           
  Branches           ?        0           
==========================================
  Hits               ?     2410           
  Misses             ?      283           
  Partials           ?        0           

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 9f35743...c860727. Read the comment docs.

SanketDG
SanketDG previously approved these changes Aug 3, 2020
Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

Looks good except for that one tiny question!

systers_portal/systers_portal/settings/testing.py Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
systers_portal/systers_portal/settings/testing.py Outdated Show resolved Hide resolved
systers_portal/systers_portal/settings/testing.py Outdated Show resolved Hide resolved
systers_portal/systers_portal/settings/testing.py Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@SanketDG
Copy link
Contributor

@satya7289 Do you want me to pick this up?

Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

Looks good, can you fix the conflicts and integrate #580's changes?

@satya7289 satya7289 closed this Aug 20, 2020
@satya7289 satya7289 reopened this Aug 20, 2020
@satya7289
Copy link
Contributor Author

satya7289 commented Aug 20, 2020

Looks good, can you fix the conflicts and integrate #580's changes?

It would be great if that pr is merged after this PR. Any way I 'll update this according.

'PASSWORD': config('DB_PASSWORD'),
'HOST': config('DB_HOST', default='localhost'),
'PORT': config('DB_PORT', default=5432, cast=int),
'NAME': config('TEST_DB_NAME', default='systersdb'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why not keep the same names?

So basically the same names for beth dev and testing, is that causing issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I wanted to keep the test DB and main DB different. Actually, This TEST_DB is required for only Github's action databases.
However, This TEST_DB must be empty on the dev environment for running test suits locally.
@SanketDG

Copy link
Contributor

@SanketDG SanketDG Aug 22, 2020

Choose a reason for hiding this comment

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

I understand but this has no effect https://docs.djangoproject.com/en/3.1/topics/testing/overview/#the-test-database

I am pretty sure the config involved is wrong, I had raised the issue before in a weekly session.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whatever name you supply, test_ will be prepended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which config are you thinking wrong..! TEST_DB or anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can even use the SQLite database for testing purpose. If we use SQLite then no need to use configure extra services for PostgreSQL in Github Actions...!

Copy link
Contributor

Choose a reason for hiding this comment

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

TEST_DB yes!

There is a reason why Django uses the same engine by default, I'd like to use the same database as used for dev (unless we will have to go through a lot of obstacles), as there is no actual significant improvement to use a different database, but lot's of things can go wrong (databases have discrepancies between them)

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please look at now, I have updated with SQLite testing DB. SQLite is a default database for Django.
I think there should no problem with using SQLite for testing purposes as far I know Django doesn't depend on which DB are you using.
Please let me know if anything went wrong if I use SQLite as testing DB.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I am not finding a good reason to go for such a discrepancy of not using the same database.

I will be happy to merge this though.

Also, can you squash your commits to be the actual issues you worked on (Github Actions and 3.8 update), otherwise I will just squash into one commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Squashing to 1 commit will be fine.

self.assertFalse(form.is_valid())
self.assertTrue(form.errors['time'],
["Time should not be a time that has already passed."])
self.assertRaises(ValidationError, form.clean_time())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand any of the changes here, and please don't remove a whole test check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you check deeply then you come to know that these are test cases for RequestMeetupFormTestCase not AddMeetupFormTestCase that's why I am adding the RequestMeetupForm instead of AddMeetupForm Actually whole test cases are wrong..

Also, Here this line self.assertRaises(ValidationError, form.clean_time()) is not needed because in the clean_time method time is used for return ValidationError(however, here time is None because on adding requestmeetup due to clean_time method time will be None). You can check these phenomenon by printing form.clean().

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for diving deep into this! Can you please open an issue about this?

Copy link
Contributor

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Thank you for showing patience here @satya7289, this was a relatively large change

@SanketDG SanketDG merged commit 6e6e139 into anitab-org:develop Aug 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants