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

increase code coverage to 93 percent #1355

Merged
merged 18 commits into from
Oct 17, 2024
Merged

increase code coverage to 93 percent #1355

merged 18 commits into from
Oct 17, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Oct 2, 2024 •
edited
Loading

Description

Boost code coverage on the API side from 91 to 93 percent.

  1. Comment out some commands we are not using at the moment and may not ever use.
  2. Write some tests for service/rest.py that were missing.
  3. Refactor some code so there is better coverage.

Security Considerations

N/A

@terrazoon terrazoon self-assigned this Oct 2, 2024
@terrazoon terrazoon marked this pull request as draft October 2, 2024 19:39
@terrazoon terrazoon changed the title break test so we can see coverage numbers increase code coverage to 93 percent Oct 3, 2024
@terrazoon terrazoon marked this pull request as ready for review October 3, 2024 18:04
ccostino
ccostino reviewed Oct 5, 2024
View reviewed changes
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @terrazoon!

I had a few questions as I was going through, and I think it's safe to remove most of the areas where you had just commented out the code - we're not going to use that!

And 🎉 on those changes to the tests, that's quite a bit!

app/aws/s3.py Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/service/rest.py Outdated Show resolved Hide resolved
app/service/rest.py Outdated Show resolved Hide resolved
notifications_utils/sanitise_text.py Show resolved Hide resolved
@terrazoon terrazoon requested a review from ccostino October 8, 2024 18:03
ccostino
ccostino approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @terrazoon!

A-Shumway42
A-Shumway42 approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@A-Shumway42 A-Shumway42 left a comment

Choose a reason for hiding this comment

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

LGTM!

@terrazoon terrazoon merged commit f4f6312 into main Oct 17, 2024
7 checks passed
@terrazoon terrazoon deleted the notify-api-1351 branch October 17, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Shumway42 A-Shumway42 A-Shumway42 approved these changes

@ccostino ccostino ccostino approved these changes

Assignees

@terrazoon terrazoon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@terrazoon @ccostino @A-Shumway42