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

Config options, solution 2 (fixes #2) #23

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

matejc
Copy link
Contributor

@matejc matejc commented Aug 11, 2021

add specific-to-handler config options to override config entries

@matejc matejc requested a review from Tattoo August 11, 2021 11:47
@matejc matejc force-pushed the feat/config-options-solution-2 branch from f5d3e12 to 17468d5 Compare August 12, 2021 14:38
@matejc matejc marked this pull request as ready for review August 13, 2021 13:43
@matejc matejc force-pushed the feat/config-options-solution-2 branch from 17468d5 to afd2873 Compare August 13, 2021 13:55
Copy link
Contributor

@Tattoo Tattoo left a comment

Choose a reason for hiding this comment

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

Few changes more, if you would be kind :)

src/oxygen/zap.py Outdated Show resolved Hide resolved
src/oxygen/zap.py Outdated Show resolved Hide resolved
tests/utest/oxygen/test_oxygen_cli.py Outdated Show resolved Hide resolved
tests/utest/zap/test_basic_functionality.py Outdated Show resolved Hide resolved
@matejc matejc force-pushed the feat/config-options-solution-2 branch from afd2873 to 0ded2a4 Compare August 25, 2021 13:01
Tattoo
Tattoo previously approved these changes Aug 25, 2021
@Tattoo Tattoo dismissed their stale review August 25, 2021 13:13

still some work to do

('result_file',): {}})

@patch('oxygen.oxygen.RobotInterface')
@patch.object(sys, 'argv', base_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the critical parts to understand from the test case and thus, it's very weird that it's not in the context of code, but outside of it in a decorator.

Also, needing to scroll up to see base_args is bit cumbersome, where it could be in the context of the code

patch.object() can be used as a context manager (example from mock documentation):

with patch.object(ProductionClass, 'method', return_value=None) as mock_method:
    thing = ProductionClass()
    thing.method(1, 2, 3)

mock_method.assert_called_once_with(1, 2, 3)

So in other words, something like this would be nice:

@patch('oxygen.oxygen.RobotInterface')
def test_cli_no_optional_arguments(self, mock_robot_iface):
     cmd = f'oxygen oxygen.zap {self.ZAP_XML}'  # here you can see what the actual command is
     with patch.object(sys, 'argv', cmd.split()):  
         self.cli.run()

    self.assertEquals(self.handler._config['accepted_risk_level'], 2)
    self.assertEquals(self.handler._config['required_confidence_level'], 1)

Comment on lines 33 to 36
expected_suite = create_autospec(TestSuite)
mock = Mock()
mock.running.build_suite = Mock(return_value=expected_suite)
mock_robot_iface.return_value = mock
Copy link
Contributor

Choose a reason for hiding this comment

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

This repeats in all tests, thus should be in setUp()

Comment on lines 43 to 53
mock.running.build_suite.assert_called_once()

expected_suite.run.assert_called_once_with(
output=str(RESOURCES_PATH / 'zap' / 'zap_robot_output.xml'),
log=None,
report=None,
stdout=ANY
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of these verifications? As far as I can see, the test is not interested in these details -- it seems they are just copy-paste?

from ..helpers import RESOURCES_PATH
from oxygen.oxygen import OxygenCLI
from robot.running.model import TestSuite
from unittest import TestCase
from unittest.mock import ANY, create_autospec, Mock, patch


import sys
Copy link
Contributor

Choose a reason for hiding this comment

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

The pep8-compliant ordering should be:

Suggested change
from ..helpers import RESOURCES_PATH
from oxygen.oxygen import OxygenCLI
from robot.running.model import TestSuite
from unittest import TestCase
from unittest.mock import ANY, create_autospec, Mock, patch
import sys
import sys
from unittest import TestCase
from unittest.mock import ANY, create_autospec, Mock, patch
from robot.running.model import TestSuite
from oxygen.oxygen import OxygenCLI
from ..helpers import RESOURCES_PATH

To explain:

# first come imports from standard library
# and first of them, imports of full modules
import sys  
# and then "from ... import"s
from unittest import TestCase  
from unittest.mock import ANY, create_autospec, Mock, patch

# we also leave one empty line to denote to others that "section" of imports has changed

# then come imports from 3rd party modules
from robot.running.model import TestSuite

# another empty line to denote "section"

# then come imports from local code
from oxygen.oxygen import OxygenCLI
# with relative imports being last because they are the 'closest' 
from ..helpers import RESOURCES_PATH

@matejc matejc force-pushed the feat/config-options-solution-2 branch from 0ded2a4 to 1d14a5b Compare August 26, 2021 09:58
@matejc matejc requested a review from Tattoo August 26, 2021 14:08
Copy link
Contributor

@Tattoo Tattoo left a comment

Choose a reason for hiding this comment

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

Excellent work 💖 '

@matejc matejc merged commit e107752 into master Sep 8, 2021
@matejc matejc deleted the feat/config-options-solution-2 branch September 8, 2021 08:06
@Tattoo
Copy link
Contributor

Tattoo commented Nov 6, 2021

fixes #2

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

2 participants