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

Created 3 tests for containers #4

Merged
merged 6 commits into from
Jun 20, 2023
Merged

Created 3 tests for containers #4

merged 6 commits into from
Jun 20, 2023

Conversation

Sacramento-20
Copy link
Contributor

@Sacramento-20 Sacramento-20 commented Jun 3, 2023

Created 3 tests for the containers for the three specified cases:

@Sacramento-20 Sacramento-20 requested a review from cunha June 3, 2023 00:49
Copy link
Member

@cunha cunha left a comment

Choose a reason for hiding this comment

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

Please check comments inline. General suggestions:

  • Use linters to check code; address lints whenever possible.
  • Autoformat the code using black.
  • Do not keep commented-out code in the Pull Request; move documentation to a README file or proper comments.
  • Extend the module so Scout works by default (it's what the module will be used for)

scout/scout.py Outdated Show resolved Hide resolved
scout/scout.py Outdated Show resolved Hide resolved
# https://docs.docker.com/engine/reference/commandline/ps/
class ContainerState(enum.StrEnum):
# # https://docs.docker.com/engine/reference/commandline/ps/
class ContainerState(enum.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

This is not equivalent to using StrEnum. Document that we should move to StrEnum if Python 3.11 is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StrEnum not compatible with the current version, the change will be included in the documentation.

scout/scout.py Outdated
@@ -34,17 +35,30 @@ class ContainerState(enum.StrEnum):
def is_done(self):
return self in [ContainerState.EXITED, ContainerState.DEAD]

# Set de configuração do scout especificamente
Copy link
Member

Choose a reason for hiding this comment

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

Do not leave commented-out code in Pull Requests. Documentation should be written in comments, README files, the Wiki.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed code comment.

scout/scout.py Outdated

@dataclasses.dataclass(frozen=True)
class ScoutConfig:
credentials_file: pathlib.Path
output_dir: pathlib.Path
docker_image: str = "rossja/ncc-scoutsuite:aws-latest"
docker_image: str = "alpine"
Copy link
Member

Choose a reason for hiding this comment

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

This is the public interface of the proposed module. The default image should be Scout, not the testing image. The testing image should be set by the test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected code, importing the scout image during module configuration.

import argparse

parser = argparse.ArgumentParser()
parser.add_argument('-t1', '--task1', type=int ,help='Task 1')
Copy link
Member

Choose a reason for hiding this comment

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

Please give each test a mnemonic name (instead of numbers).

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we can just run all tests every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Provided detailed actions for argparse. Initially, the tests are not executing together, but I will create a module for that.

@@ -17,14 +25,39 @@
def callback(label: str, success: bool) -> None:
logging.info("Callback for %s running, success=%s", label, success)

def task_1(s, taskcfg, seconds: int):
task_generic_sleep = ["sleep", f'{1}']
logging.info("")
Copy link
Member

Choose a reason for hiding this comment

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

No empty lines and no ALL CAPS in the logs, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line logging.info("") removed.

@@ -17,14 +25,39 @@
def callback(label: str, success: bool) -> None:
logging.info("Callback for %s running, success=%s", label, success)

def task_1(s, taskcfg, seconds: int):
task_generic_sleep = ["sleep", f'{1}']
Copy link
Member

Choose a reason for hiding this comment

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

Please use linters to check for unnecessary complexity such as f"{1}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!

task_generic_sleep = ["sleep", f'{1}']
logging.info("")
logging.info("RUNNING TASK 1")
s.enqueue(taskcfg, task_generic_sleep, seconds)
Copy link
Member

Choose a reason for hiding this comment

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

We should generate multiple test containers (5 is what I had planned in the issue). Also, sleep 1 is way faster and won't allow us to check whether code is correctly handling the containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected and included in task_1.

s.shutdown(False)

def task_3(s, taskcfg, seconds: int):
task_generic_error = ["sh", "-c", f'sleep {seconds}', "&&" , "exit", "1"]
Copy link
Member

Choose a reason for hiding this comment

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

The parameters to sh are -c and f"sleep {seconds} && exit 1", so the latter should be a single entry in the list. Replace exit 1 with false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK !

Copy link
Member

@cunha cunha left a comment

Choose a reason for hiding this comment

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

Please check comments inline. General suggestions:

  • Use linters to check code; address lints whenever possible.
  • Autoformat the code using black.
  • Do not keep commented-out code in the Pull Request; move documentation to a README file or proper comments.
  • Extend the module so Scout works by default (it's what the module will be used for)

@Sacramento-20 Sacramento-20 requested a review from cunha June 13, 2023 19:04
scout/scout.py Outdated
@@ -39,7 +39,7 @@ def is_done(self):
class ScoutConfig:
credentials_file: pathlib.Path
output_dir: pathlib.Path
docker_image: str = "rossja/ncc-scoutsuite:aws-latest"
Copy link
Member

Choose a reason for hiding this comment

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

We should keep the default image as the default value for docker_image. This can be overwritten in the tests as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK!

scout/scout.py Outdated
outfp = self.config.output_dir / cfg.label
with gzip.open(outfp / "result.json.gz", "wt", encoding="utf8") as fd:
json.dump(r, fd)
self.task_completion_callback(cfg.label, True)
logging.info(
"Scout run completed, id %s status %d",
"Scout run completed, id %s sta+-tus %d",
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved and will be included in the next commit.

@cunha cunha merged commit fa61c4d into TopologyMapping:main Jun 20, 2023
@cunha cunha deleted the branch_test_container branch June 20, 2023 16:12
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.

2 participants