Skip to content

CV3-77 Clean up base.py test file #514

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
Open
mifeille wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

CV3-77 Clean up base.py test file #514

mifeille wants to merge 1 commit into from

Conversation

@mifeille
Copy link
Contributor

@mifeille mifeille commented Nov 26, 2019 •
edited
Loading

Description

The base.py file has a large amount of test data that is loaded before tests are run. Clean up this file by putting this data is a separate data file that is loaded by base.py.

Type of change

Please select the relevant option

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Others (cosmetics, styling, improvements)
  • This change requires a documentation update

Checklist

  • My changes generate no new warnings
  • My code follows the style guidelines of this project
  • I have linted my code prior to submission
  • Existing unit tests pass locally with my changes
  • Implementation works according to expectations

How to Test

  • Clean base.py file
  • Check if existing unit tests pass with my changes

JIRA

CV3-77

@mifeille mifeille added the wip label Nov 26, 2019
@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch 11 times, most recently from cedf3d2 to b6d4637 Compare December 3, 2019 12:16
@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch 13 times, most recently from 96d7677 to 9a596f6 Compare December 10, 2019 07:25
@mifeille mifeille added peer review and removed wip labels Dec 10, 2019
MCFrank16
MCFrank16 approved these changes Dec 10, 2019
View reviewed changes
Copy link

@MCFrank16 MCFrank16 left a comment

Choose a reason for hiding this comment

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

good job @mifeille it looks fine for me, but there are some files where you forgot to put on a new line.

@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch from 9a596f6 to 872705b Compare December 10, 2019 08:42
@mifeille
Copy link
Contributor Author

mifeille commented Dec 10, 2019

good job @mifeille it looks fine for me, but there are some files where you forgot to put on a new line.

Thank you @MCFrank16 It is done!

shaluchandwani
shaluchandwani suggested changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@shaluchandwani shaluchandwani left a comment

Choose a reason for hiding this comment

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

Good Work on the PR @mifeille, It is working as expected. I have just one doubt that - should Kigali be added as locations in fixtures? I have commented on the same please check.

@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch 4 times, most recently from c96dae9 to f58f8f1 Compare December 10, 2019 11:14
shaluchandwani
shaluchandwani approved these changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@shaluchandwani shaluchandwani left a comment

Choose a reason for hiding this comment

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

Good work on the changes suggested @mifeille.

@mifeille mifeille added TTL Review Technical Team Lead Review and removed peer review labels Dec 10, 2019
@mifeille mifeille requested a review from joshuaocero December 10, 2019 12:00
joshuaocero
joshuaocero suggested changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@joshuaocero joshuaocero left a comment

Choose a reason for hiding this comment

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

Well done @mifeille. Please see my comments though. I basically think it would be better to handle association data the same way we handle the other data. The two functions could, therefore, be merged. Also for your data json files, since the folder is called test_data, the file names do not need to have data prepended. We can remove the repetition.

tests/base.py Outdated
color='green',
description='The description')
tag.save()
files = [
Copy link
Contributor

@joshuaocero joshuaocero Dec 10, 2019

Choose a reason for hiding this comment

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

Aren't we able to get all files in the test_data folder so we do not have edit this file when we have new test data to insert into the database?

tests/base.py Outdated
resolved=False,
)
response_1.save()
association_tables_files = [
Copy link
Contributor

@joshuaocero joshuaocero Dec 10, 2019

Choose a reason for hiding this comment

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

Can we name the association table data differently so it can be easily identifiable?

tests/base.py
f.write('Traceback (most recent call last):\r')
f.write('if pattern.search(line):\r')
insert_data_in_database(db_session, files)
insert_association_tables_data(db_session, association_tables_files)
Copy link
Contributor

@joshuaocero joshuaocero Dec 10, 2019

Choose a reason for hiding this comment

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

These two functions appear to be pretty similar. You should consider merging them.

@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch 5 times, most recently from 96c0f7b to b4f0c46 Compare December 11, 2019 13:53
CV3-77-story(base.py): clean up base.py test file
ce1bb4b 
- clean up this file by putting test data in a separate data file that is loaded by base.py

[Delivers CV3-77]
@mifeille mifeille force-pushed the story/CV3-77-clean-up-base branch from b4f0c46 to ce1bb4b Compare December 11, 2019 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@joshuaocero joshuaocero joshuaocero requested changes

@MCFrank16 MCFrank16 MCFrank16 approved these changes

@shaluchandwani shaluchandwani shaluchandwani approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

TTL Review Technical Team Lead Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mifeille @joshuaocero @MCFrank16 @shaluchandwani