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

[Issue 943] Cleanup various API components, improve test coverage #944

Merged
merged 3 commits into from
Jan 2, 2024

Conversation

chouinar
Copy link
Collaborator

@chouinar chouinar commented Dec 21, 2023

Summary

Fixes #943

Time to review: 10 mins

Changes proposed

Improved test coverage from 84% to 92%

Add audit log tests to test coverage

Fix audit log tests to not try to commit code

Remove miscellaneous unused components in the Makefile

Adjust the location of several tests which were nested in a different directory than the rest of the tests.

Add various utility tests

Context for reviewers

This is a random assortment of little things. I started by fixing up some of the paths, and then noticed our test coverage was lower than I expected, so fixed the configurations / tests.

Most of these tests I didn't write here, and puled from other projects based on the template repo - need to backport those tests into the template as they shouldn't be missing there.

Additional information

make test-coverage will run the tests with branch coverage, then running make test-coverage-report will pull up an HTML page with the actual coverage metrics line-by-line which I used to figure out what wasn't being tested.

@@ -231,9 +233,6 @@ lint-security: # https://bandit.readthedocs.io/en/latest/index.html
cmd: ## Run Flask app CLI command (Run `make cli args="--help"` to see list of CLI commands)
$(FLASK_CMD) $(args)

cmd-user-create-csv: ## Create a CSV of the useres in the database (Run `make cli-user-create-csv args="--help"` to see the command's options)
Copy link
Collaborator

@acouch acouch Dec 29, 2023

Choose a reason for hiding this comment

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

Thanks for removing this and the sleep target!

Copy link
Collaborator

@acouch acouch left a comment

Choose a reason for hiding this comment

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

(praise) nice!
looks like some of this could be committed upstream. We could make an issue if you feel so inspired.

@chouinar chouinar merged commit 0e2c693 into main Jan 2, 2024
13 checks passed
@chouinar chouinar deleted the chouinar/943-cleanup branch January 2, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Various cleanup tasks for the API
2 participants