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

[Test] Test the get_discoveries cli command #166

Merged
merged 99 commits into from
Aug 19, 2021

Conversation

alaabenfatma
Copy link
Contributor

This PR targets

Note: Upon the merge of this PR, the previous PR (#164 ) can be closed since this one builds upon it. However, the latter PR can also be safely merged before this one as well (no conflicts will be raised nonetheless).

Feats:

  • Test the CLI's ability to retrieve/export all discoveries
  • Test the CLI's ability to retrieve/export discoveries based on
    • Specified state
    • Specified filename
  • Test the CLI's ability to export discoveries as CSV without NaN fields

@marcorosa
Copy link
Member

Hi @alaabenfatma, tests are failing so please fix the code. If tests fail I cannot start the code review

tests/functional_tests/test_get_discoveries.py Outdated Show resolved Hide resolved
tests/functional_tests/test_get_discoveries.py Outdated Show resolved Hide resolved
tests/functional_tests/test_get_discoveries.py Outdated Show resolved Hide resolved
tests/functional_tests/test_get_discoveries.py Outdated Show resolved Hide resolved
tests/functional_tests/test_get_discoveries.py Outdated Show resolved Hide resolved
@marcorosa
Copy link
Member

The tests are failing. The possible reason (my guess) is that when you try to add discoveries you're not adding anything since the inserts break db rules, i.e., you can't add a discovery if you don't have the rule and the repo it references.

@alaabenfatma alaabenfatma requested a review from marcorosa August 17, 2021 14:44
Copy link
Member

@marcorosa marcorosa left a comment

Choose a reason for hiding this comment

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

I have 2 minor comments on style. Once fixed we can merge

discoveries = []
discoveries_count = 5
for state in ['new', 'false_positive', 'addressing',
'not_relevant', 'fixed']:
Copy link
Member

Choose a reason for hiding this comment

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

wrong indentation (they should start below "new")


@classmethod
def tearDownClass(cls):
# Remove the repo and all its discoveries
Copy link
Member

Choose a reason for hiding this comment

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

convert this into a docstring

discoveries = []
discoveries_count = 5
for state in ['new', 'false_positive', 'addressing',
'not_relevant', 'fixed']:
Copy link
Member

Choose a reason for hiding this comment

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

same comment as in the other file

@marcorosa marcorosa merged commit bfd8a57 into develop Aug 19, 2021
@marcorosa marcorosa deleted the test/get_discoveries_cli branch August 19, 2021 20:25
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.

3 participants