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

Feature/issue 623 add wa sv2 for export/reports download #623

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

fullofentropy
Copy link

@fullofentropy fullofentropy commented Sep 14, 2022

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #623

Type of change

Added

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test Code used attached
  • Executed Regression tests

Test Configuration: Kali Linux VM with cloud.tenable.com APIs

  • Python Version(s) 3.8:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@fullofentropy fullofentropy requested a review from a team September 14, 2022 09:59
@fullofentropy
Copy link
Author

My apologies, I clicked the button too soon to start a pull request, I have done my own unit testing but haven't figured out regression tests for this project.

CHANGELOG.md Outdated Show resolved Hide resolved
tenable/io/scans.py Outdated Show resolved Hide resolved
tenable/io/scans.py Outdated Show resolved Hide resolved
tenable/io/scans.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aseemsavio aseemsavio left a comment

Choose a reason for hiding this comment

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

Hello @fullofentropy, Thanks for the PR. I've added a few comments. In addition, please add tests confirming the behaviour.

@aseemsavio
Copy link
Collaborator

Hi! If this PR fixes #622, please ensure it's properly written in the PR description (without space between "#" and 622). Thanks!

@fullofentropy fullofentropy changed the title Feature/issu 622 add wa sv2 for export/reports download Feature/issue 623 add wa sv2 for export/reports download Oct 26, 2022
@fullofentropy
Copy link
Author

fullofentropy commented Oct 26, 2022

@aseemsavio, I will add some pytests to confirm the behavior, question below as well.

Hello @fullofentropy, Thanks for the PR. I've added a few comments. In addition, please add tests confirming the behaviour.

image

still need dummy test data
corrected discription
this needs to be updated by adding a WAS test data to request a report with
@fullofentropy
Copy link
Author

So in the current feature branch, I have added 3 pytest unit tests that currently fail because I do not know of a WAS scan_id or history_id I can use for testing. Could someone assist me with this? I also installed the html report (in the zip) generator for the pytest execution on this branch.

The regression test was ran and passed using the fork, it is my added (and incomplete) tests that are failing.
pytest_report_issue_report.zip

Note: This is the first time I am using pytest, so I spent some time learning it and some of the extended features this project uses.

@fullofentropy
Copy link
Author

FYI: I did test the implementation using my own tenable credentials and WAS based scans.

@aseemsavio
Copy link
Collaborator

Hi @fullofentropy. I'll try and explain as best I can.

You must have seen your test function accepting api as an argument. The pytest fixture named api injects a TenableIO object with the credentials mentioned into all the test functions that accepts a param named api.

As you may be familiar, pytest records the HTTP request and response in the cassette (yaml) files, so that it can use the recorded response during the subsequent test runs instead of actually making HTTP calls every time.

To achieve this, you need to put valid credentials in the api fixture function and run it once (locally before committing the code). This will call the APIs with your credentials; record the HTTP responses; and creates cassette files. The next time you run the test, the APIs won't be called. Instead, the response will be fetched from the cassette files. Note that this won't store your credentials in the cassette files.

You must make sure to remove your credential from the api fixture before you commit your changes. You need to also commit the generated cassette files.

Hope this helps. Let me know if you need any more information.

@SteveMcGrath SteveMcGrath requested a review from a team as a code owner June 18, 2024 16:18
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.

None yet

3 participants