feat: Support pending Pacts #171

m-aciek · 2020-09-03T15:34:50Z

Closes #164.

To conform to contributing guideline. Closes pact-foundation#151.

Most probably rebasing artifact.

matthewbalvanz-wf · 2020-09-03T22:18:06Z

@m-aciek mock should be part of the Python standard library from 3.3 and above, maybe we could remove that entirely now that support for Python 2.7 has been removed?
https://docs.python.org/3/library/unittest.mock.html

m-aciek · 2020-09-03T23:06:29Z

@matthewbalvanz-wf Unfortunately in 3.4 and 3.5 mock that is shipped with unittest has different API than in 3.8 so the asserts in VerifyWrapper tests fail.

przemyslaw-jakielaszek · 2020-09-04T09:09:26Z

pact/cli/verify.py

 @click.option(
    'log_dir', '--log-dir',
    help='The directory for the pact.log file.')
 @click.option(
    'log_level', '--log-level',
    help='The logging level.')
+@click.option(
+    'enable_pending', '--enable-pending/--no-enable-pending',


How about 'pending', '--pending/--no-pending',default=False
is_flag=True will be passed implicitly if a boolean flag is used.

I wanted to replicate the flag that is used in underlying CLI and to be in line with flag name used in documentation https://docs.pact.io/pact_broker/advanced_topics/pending_pacts/#to-start-using-the-pending-pacts-feature.

I used default=None also to replicate usage of underlying CLI so to map False -> --enable…, True -> --no-enable…, None -> no flag.

you are right. It is said to use enable-pending convention: Master issue for new 'pending pacts' feature pact_broker#320

it is also said that flag should be set to False as a default value. Moreover I do not see a case when no flag differs from no-enable-pending

Ok, so to be in line with the guidelines you linked and to make the implementation minimal, I will stick only with positive flag, set False as default and do not add --no-enable-pending to underlying library.

przemyslaw-jakielaszek · 2020-09-04T09:46:00Z

pact/verify_wrapper.py

@@ -199,3 +200,11 @@ def publish_results(self, provider_app_version, command):
        command.extend(["--provider-app-version",
                       provider_app_version,
                       "--publish-verification-results"])
+
+    @classmethod
+    def _get_enable_pending_flag(cls, flag_value):


How about

def _get_boolean_flag(self, flags, flag_name): flag_value = flags.get(flag_name) if flag_value is True: return ['--' + flag_name] elif flag_value is False: return ['--no-' + flag_name] return []

There is no such a flag as --no-verbose, and for just one case I think it is a premature optimisation.

przemyslaw-jakielaszek · 2020-09-04T13:31:07Z

pact/verify_wrapper.py

@@ -169,6 +169,7 @@ def call_verify(self, *pacts, provider_base_url, provider, **kwargs):

        if(kwargs.get('verbose', False) is True):
            command.extend(['--verbose'])
+        command.extend(self._get_enable_pending_flag(kwargs.get('enable_pending')))


_get_boolean_flag('pending')

There is no such a flag as --no-verbose, and for just one case I think it is a premature optimisation.

Maybe I am wrong, but I think there is a no-verbose flag.
https://github.com/pact-foundation/pact-python/pull/171/files#diff-99dbf564da4f139a45a61bf9c28b4f28R104

It is lacking in the ruby verifier help message (see https://github.com/pact-foundation/pact-provider-verifier/blob/master/README.md), but I checked and indeed this ruby binary accepts --no-verbose. Although according to #171 (comment) I'll drop the negative flag passing.

przemyslaw-jakielaszek · 2020-09-04T13:31:45Z

pact/verify_wrapper.py

@@ -169,6 +169,7 @@ def call_verify(self, *pacts, provider_base_url, provider, **kwargs):

        if(kwargs.get('verbose', False) is True):


command.extend(self. _get_boolean_flag('verbose'))

There is no such a flag as --no-verbose, and for just one case I think it is a premature optimisation.

m-aciek · 2020-09-04T14:46:18Z

@matthewbalvanz-wf Summary of failing tests because of mock in 3.5 compatibility issues:

Error
Traceback (most recent call last):
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 605, in run
    testMethod()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 1159, in patched
    return func(*args, **keywargs)
  File "/Users/maciek/projects/pact-python/tests/cli/test_verify.py", line 122, in test_broker_url_and_provider_required
    mock_wrapper.assert_called()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 585, in __getattr__
    raise AttributeError(name)
AttributeError: assert_called

Error
Traceback (most recent call last):
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 605, in run
    testMethod()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 1159, in patched
    return func(*args, **keywargs)
  File "/Users/maciek/projects/pact-python/tests/cli/test_verify.py", line 339, in test_expand_directories_called
    mock_expand_dirs.assert_called_once()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 585, in __getattr__
    raise AttributeError(name)
AttributeError: assert_called_once

Error
Traceback (most recent call last):
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 605, in run
    testMethod()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 1159, in patched
    return func(*args, **keywargs)
  File "/Users/maciek/projects/pact-python/tests/test_verifier.py", line 74, in test_expand_directories_called_for_pacts
    mock_expand_dir.assert_called_once()
  File "/Users/maciek/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 585, in __getattr__
    raise AttributeError(name)
AttributeError: assert_called_once

SubTest failure: Traceback (most recent call last):
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 523, in subTest
    yield
  File "/…/projects/pact-python/tests/test_verifier.py", line 84, in test_passes_enable_pending_flag_value
    mock_wrapper.call_args.kwargs,
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 682, in assertTrue
    raise self.failureException(msg)
AssertionError: False is not true : kwargs

Error
Traceback (most recent call last):
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/case.py", line 605, in run
    testMethod()
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 1159, in patched
    return func(*args, **keywargs)
  File "/…/pact-python/tests/test_verify_wrapper.py", line 160, in test_rerun_command_called
    mock_rerun_cmd.assert_called_once()
  File "/…/.pyenv/versions/3.5.9/lib/python3.5/unittest/mock.py", line 585, in __getattr__
    raise AttributeError(name)
AttributeError: assert_called_once

matthewbalvanz-wf · 2020-09-04T14:59:33Z

Looks like assert_called and assert_called_once were added in 3.6, so it seems like using the library until that is end of life is the path forward.

przemyslaw-jakielaszek · 2020-09-04T15:03:49Z

pact/verify_wrapper.py

@@ -169,6 +169,7 @@ def call_verify(self, *pacts, provider_base_url, provider, **kwargs):

        if(kwargs.get('verbose', False) is True):
            command.extend(['--verbose'])
+        command.extend(self._get_enable_pending_flag(kwargs.get('enable_pending')))


Maybe I am wrong, but I think there is a no-verbose flag.
https://github.com/pact-foundation/pact-python/pull/171/files#diff-99dbf564da4f139a45a61bf9c28b4f28R104

przemyslaw-jakielaszek · 2020-09-04T15:14:28Z

pact/cli/verify.py

 @click.option(
    'log_dir', '--log-dir',
    help='The directory for the pact.log file.')
 @click.option(
    'log_level', '--log-level',
    help='The logging level.')
+@click.option(
+    'enable_pending', '--enable-pending/--no-enable-pending',


you are right. It is said to use enable-pending convention: Master issue for new 'pending pacts' feature pact_broker#320

it is also said that flag should be set to False as a default value. Moreover I do not see a case when no flag differs from no-enable-pending

m-aciek · 2020-09-07T12:41:12Z

I've added Python 3.4 syntax compatibility and simplified code. Ready for review again.

Closes pact-foundation#164

m-aciek · 2020-09-07T15:58:08Z

I've fixed code issue with Click option.

bethesque · 2020-09-07T23:00:51Z

Would you mind leaving it as --enable-pending and --no-enable-pending in the python interface? When trying to write documentation that is applicable for all languages, it makes it very difficult when each of the 11 languages chooses a slightly different option name!

elliottmurray · 2020-09-08T08:44:45Z

@bethesque isn't the 'no' option redundant? Just wondering if it should be default off and you just ask --enable-pending but not sure what the plans are for this ongoing?

m-aciek · 2020-09-09T06:16:14Z

@bethesque @elliottmurray I had the same doubt and simplified implementation to avoid redundancy.

The use case for --no-enable-pending that I can think of would be having some default configuration (e.g. in pyproject.toml) that could be overridden in a command call.

I plan to implement support for --no-enable-pending on another branch and open new alternative pull request to avoid going round in circles in case of further discussion.

m-aciek · 2020-09-09T07:07:39Z

@bethesque @elliottmurray

The use case for --no-enable-pending that I can think of would be having some default configuration (e.g. in pyproject.toml) that could be overridden in a command call.

In this use case default for enable-pending flag would be None, meaning „not overriding some default configuration” in case of no of those flags are added. I'm assuming this in the implementation.

elliottmurray · 2020-09-09T09:02:48Z

I'm going to merge this and do a release later. It doesn't harm as far as I can tell and we can add the extra flag in another PR

bethesque · 2020-09-10T02:33:48Z

Sorry, I don't care about --no-enable-pending, just the --enable-pending (rather than --pending or whatever it was)

m-aciek force-pushed the enable-pending branch from 4f35751 to b2828ab Compare September 3, 2020 15:41

Maciej Olko added 2 commits September 3, 2020 17:42

ci(pre-commit): add commitizen to pre-commit configuration

fbbd5fa

To conform to contributing guideline. Closes pact-foundation#151.

fix(verifier): remove superfluous option from verify CLI command

fc6c365

Most probably rebasing artifact.

m-aciek force-pushed the enable-pending branch 3 times, most recently from 915e278 to 80e90f6 Compare September 3, 2020 15:59

m-aciek closed this Sep 3, 2020

m-aciek reopened this Sep 3, 2020

m-aciek marked this pull request as draft September 4, 2020 06:54

m-aciek force-pushed the enable-pending branch 2 times, most recently from e256b9d to a344f3d Compare September 4, 2020 10:48

m-aciek marked this pull request as ready for review September 4, 2020 10:49

przemyslaw-jakielaszek suggested changes Sep 4, 2020

View reviewed changes

feat(verifier): add enable_pending argument handling in verify wrapper

39de1f3

m-aciek force-pushed the enable-pending branch from a344f3d to b444924 Compare September 5, 2020 12:48

m-aciek and others added 2 commits September 5, 2020 19:07

test: bump mock to 3.0.5

d51c88d

feat(verifier): pass enable_pending flag in Verifier's methods

2b57446

m-aciek force-pushed the enable-pending branch 5 times, most recently from 8247424 to dddc746 Compare September 7, 2020 12:39

Maciej Olko added 2 commits September 7, 2020 17:55

feat(verifier): support --enable-pending flag in CLI

e6c9ed0

Closes pact-foundation#164

refactor(verifier): add enable_pending to signature of verify methods

c64bec1

fix(verifier): remove superfluous verbose mentions

e840587

m-aciek force-pushed the enable-pending branch from dddc746 to e840587 Compare September 7, 2020 15:56

elliottmurray merged commit 46372c7 into pact-foundation:master Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support pending Pacts #171

feat: Support pending Pacts #171

m-aciek commented Sep 3, 2020

matthewbalvanz-wf commented Sep 3, 2020

m-aciek commented Sep 3, 2020 •

edited

Loading

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 4, 2020

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 5, 2020

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 4, 2020 •

edited

Loading

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 4, 2020

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 5, 2020

przemyslaw-jakielaszek Sep 4, 2020

m-aciek Sep 4, 2020

m-aciek commented Sep 4, 2020

matthewbalvanz-wf commented Sep 4, 2020

przemyslaw-jakielaszek Sep 4, 2020

przemyslaw-jakielaszek Sep 4, 2020

m-aciek commented Sep 7, 2020

m-aciek commented Sep 7, 2020

bethesque commented Sep 7, 2020

elliottmurray commented Sep 8, 2020

m-aciek commented Sep 9, 2020 •

edited

Loading

m-aciek commented Sep 9, 2020

elliottmurray commented Sep 9, 2020

bethesque commented Sep 10, 2020

		@@ -169,6 +169,7 @@ def call_verify(self, pacts, provider_base_url, provider, *kwargs):

		if(kwargs.get('verbose', False) is True):

feat: Support pending Pacts #171

feat: Support pending Pacts #171

Conversation

m-aciek commented Sep 3, 2020

matthewbalvanz-wf commented Sep 3, 2020

m-aciek commented Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-aciek Sep 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-aciek commented Sep 4, 2020

matthewbalvanz-wf commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-aciek commented Sep 7, 2020

m-aciek commented Sep 7, 2020

bethesque commented Sep 7, 2020

elliottmurray commented Sep 8, 2020

m-aciek commented Sep 9, 2020 • edited Loading

m-aciek commented Sep 9, 2020

elliottmurray commented Sep 9, 2020

bethesque commented Sep 10, 2020

m-aciek commented Sep 3, 2020 •

edited

Loading

m-aciek Sep 4, 2020 •

edited

Loading

m-aciek commented Sep 9, 2020 •

edited

Loading