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

Added tests for filters.py in sapp/ui #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

esiebomaj
Copy link
Contributor

Pre-submission checklist

  • I've ran the following linters locally and fixed lint errors related to the files I modified in this PR
    • black .
    • usort format .
    • flake8
  • I've installed dev dependencies pip install -r requirements-dev.txt and completed the following:
    • I've ran tests with ./scripts/run-tests.sh and made sure all tests are passing

Summary

The PR adds tests for sapp/frontend/ui/filters.py and increases the coverage for that file from 0% to 80%

Test Plan

to run the tests in test_run.py
python -m coverage run -m unittest sapp.ui.tests.filter_test && python -m coverage report --show-missing --skip-covered --skip-empty

Name                        Stmts   Miss  Cover   Missing
---------------------------------------------------------
sapp/ui/filters.py          137    47    80%   

Relevant Issue : mlh-fellowship/sapp #11

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 19, 2021
@facebook-github-bot
Copy link
Contributor

@grievejia has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@esiebomaj
Copy link
Contributor Author

while trying to fix some typing issues in the tests I noticed that the delete_filters function in sapp\ui\filters.py takes an argument filter_names of type Tuple[str]. This implies that it should receive a tuple with only one string in it.

But the name delete_filters implies that it should be able to accept more than one filter name. The logic in the function is also for deleting more than 1 filters
Moreover, there is already a delete_filter function that deletes just one filter

So in this commit (01c3f4d) I changed the type annotation for filter_names argument from Tuple[str] with Tuple[str, ...]
to reflect that the argument takes multiple filter_names

@esiebomaj esiebomaj marked this pull request as ready for review November 20, 2021 10:40
@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@0xedward has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants