ipatests: add missing automember-cli tests #6718

mbhalodi · 2023-02-28T09:08:57Z

This patch is focusing on negative test scenarios of automember-cli.

Related: https://pagure.io/freeipa/issue/9332

Signed-off-by: mbhalodi mbhalodi@redhat.com

rcritten · 2023-03-14T12:04:10Z

ipatests/test_xmlrpc/test_automember_plugin.py

+            badregextype_regex="qa[0-9]+.example.com")
+        with raises_exact(errors.NotFound(
+                reason=u'no such option: --badregextype-regex')):
+            command()


This class is testing CLI parsing, not the automember plugin itself. It's fine in a way to exercise the parsing code but it isn't specific to this.

ipatests/test_xmlrpc/test_automember_plugin.py

miskopo · 2023-03-14T13:32:44Z

You can find the test failures here (It's little bit tricky to navigate in Azure)

miskopo

Please see the in-line comments. The main problem is with incorrect Error type and missing/exists requirement

ipatests/test_xmlrpc/test_automember_plugin.py

flo-renaud

Hi @mbhalodi
to make the code easier to read, I would structure it with fewer classes. For instance, use a single class for all the negative tests related to automember-find, use another one for automember-show etc...
Inside each class, add functions to test each parameter (with non existent group, with bad type ...)

ipatests/test_xmlrpc/test_automember_plugin.py

flo-renaud

Hi @mbhalodi
please find a few inline comments

ipatests/test_xmlrpc/test_automember_plugin.py

mbhalodi · 2023-03-27T08:44:18Z

To note that, to resolve the Tox issue, Flo made a separate PR #6758.
Once that PR gets merged, I will rebase on top of the master branch.

rcritten · 2023-03-28T15:04:53Z

The tox failure is being addressed in: #6758

The GATING_upgrade test is unfortunately a bit flaky at times and sometimes requires multiple re-runs for it to pass. I'd wait until is fixed and you rebase on top of that to attempt multiple re-runs because you'd have to do it all again later anyway.

flo-renaud · 2023-03-29T08:48:18Z

@mbhalodi PR#6758 has been merged and should fix the tox issue. Please rebase on top of the master branch.

flo-renaud

Hi @mbhalodi
Thanks for the update. Please find my inline comments.

flo-renaud · 2023-03-29T14:25:13Z

ipatests/test_xmlrpc/test_automember_plugin.py

+        """ Try to find a rule with non-existent group/hostgroup """
+        grp = request.getfixturevalue(grp)
+        automember_grp = request.getfixturevalue(automember_grp)
+        grp.ensure_missing()


It should be automember_grp.ensure_missing().
The function can be simplified and use only automember_grp + request parameters (grp is not useful here).

ipatests/test_xmlrpc/test_automember_plugin.py

flo-renaud · 2023-03-30T14:03:31Z

@mbhalodi
pycodestyle and lint detected the following issues:

./ipatests/test_xmlrpc/test_automember_plugin.py:220:81: E501 line too long (84 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:226:81: E501 line too long (87 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:254:81: E501 line too long (82 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:293:81: E501 line too long (82 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:299:81: E501 line too long (87 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:520:81: E501 line too long (84 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:527:81: E501 line too long (87 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:696:54: E128 continuation line under-indented for visual indent
./ipatests/test_xmlrpc/test_automember_plugin.py:703:81: E501 line too long (88 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:712:54: E128 continuation line under-indented for visual indent
./ipatests/test_xmlrpc/test_automember_plugin.py:719:81: E501 line too long (88 > 80 characters)
./ipatests/test_xmlrpc/test_automember_plugin.py:728:51: E128 continuation line under-indented for visual indent
./ipatests/test_xmlrpc/test_automember_plugin.py:743:51: E128 continuation line under-indented for visual indent

and

************* Module ipatests.test_xmlrpc.test_automember_plugin
ipatests/test_xmlrpc/test_automember_plugin.py:733: [W0612(unused-variable), TestAutomemberAddConditionNegative.test_create_inclusive_with_existent_automember] Unused variable 'command')
ipatests/test_xmlrpc/test_automember_plugin.py:748: [W0612(unused-variable), TestAutomemberAddConditionNegative.test_create_exclusive_with_existent_automember] Unused variable 'command')

flo-renaud

Hi @mbhalodi
thanks for the update, we're almost done!
I have a few nitpicks related to the comments but otherwise the code LGTM.

ipatests/test_xmlrpc/test_automember_plugin.py

Revisit the bash tests and port the valid tests to upstream. Related: https://pagure.io/freeipa/issue/9332 Signed-off-by: mbhalodi <mbhalodi@redhat.com>

flo-renaud · 2023-04-04T14:12:42Z

master:

4119e4e ipatests: add missing automember-cli tests

mbhalodi force-pushed the automember branch 11 times, most recently from d693ab5 to fe9b292 Compare March 9, 2023 09:50

mbhalodi marked this pull request as ready for review March 10, 2023 05:07

mbhalodi changed the title ~~[WIP] ipatests: add missing automember-cli tests~~ ipatests: add missing automember-cli tests Mar 10, 2023

mbhalodi force-pushed the automember branch from fe9b292 to 3af3fc2 Compare March 10, 2023 06:22

abbra force-pushed the master branch from 44132f1 to 2aeb963 Compare March 10, 2023 06:42

mbhalodi force-pushed the automember branch 3 times, most recently from c86cc61 to 1331a0a Compare March 13, 2023 11:19

menonsudhir added the WIP Work in progress - not ready yet for review label Mar 13, 2023

rcritten reviewed Mar 14, 2023

View reviewed changes

ipatests/test_xmlrpc/test_automember_plugin.py Outdated Show resolved Hide resolved

miskopo self-requested a review March 14, 2023 13:33

miskopo requested changes Mar 14, 2023

View reviewed changes

flo-renaud reviewed Mar 14, 2023

View reviewed changes

mbhalodi force-pushed the automember branch 2 times, most recently from fce90f0 to 4501f6c Compare March 15, 2023 19:22

flo-renaud reviewed Mar 16, 2023

View reviewed changes

mbhalodi force-pushed the automember branch 2 times, most recently from e26bcb4 to 47ceedc Compare March 24, 2023 18:16

mbhalodi force-pushed the automember branch from e462a34 to ae03ccf Compare March 27, 2023 08:48

amore17 added the re-run Trigger a new run of PR-CI label Mar 27, 2023

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Mar 27, 2023

mbhalodi force-pushed the automember branch from ae03ccf to 31d2df3 Compare March 29, 2023 09:55

flo-renaud reviewed Mar 29, 2023

View reviewed changes

mbhalodi force-pushed the automember branch from 31d2df3 to 258f69a Compare March 30, 2023 10:38

mbhalodi force-pushed the automember branch 4 times, most recently from 9806ebf to 15275f6 Compare March 31, 2023 10:19

flo-renaud reviewed Mar 31, 2023

View reviewed changes

mbhalodi force-pushed the automember branch from 15275f6 to 62a3563 Compare March 31, 2023 17:40

amore17 added needs review Pull Request is waiting for a review ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 and removed WIP Work in progress - not ready yet for review labels Apr 3, 2023

ipatests: add missing automember-cli tests

1140d40

Revisit the bash tests and port the valid tests to upstream. Related: https://pagure.io/freeipa/issue/9332 Signed-off-by: mbhalodi <mbhalodi@redhat.com>

mbhalodi force-pushed the automember branch from 62a3563 to 1140d40 Compare April 3, 2023 10:13

flo-renaud added ack Pull Request approved, can be merged pushed Pull Request has already been pushed and removed needs review Pull Request is waiting for a review labels Apr 3, 2023

flo-renaud closed this Apr 4, 2023

This was referenced Apr 4, 2023

[Backport][ipa-4-10] ipatests: add missing automember-cli tests #6770

Closed

[Backport][ipa-4-9] ipatests: add missing automember-cli tests #6771

Closed

mbhalodi mentioned this pull request Apr 5, 2023

ipatests: add remove automember condition tests #6773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipatests: add missing automember-cli tests #6718

ipatests: add missing automember-cli tests #6718

mbhalodi commented Feb 28, 2023 •

edited

rcritten Mar 14, 2023

miskopo commented Mar 14, 2023

miskopo left a comment

flo-renaud left a comment

flo-renaud left a comment

mbhalodi commented Mar 27, 2023

rcritten commented Mar 28, 2023

flo-renaud commented Mar 29, 2023

flo-renaud left a comment

flo-renaud Mar 29, 2023

flo-renaud commented Mar 30, 2023

flo-renaud left a comment

flo-renaud commented Apr 4, 2023

ipatests: add missing automember-cli tests #6718

ipatests: add missing automember-cli tests #6718

Conversation

mbhalodi commented Feb 28, 2023 • edited

rcritten Mar 14, 2023

Choose a reason for hiding this comment

miskopo commented Mar 14, 2023

miskopo left a comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

mbhalodi commented Mar 27, 2023

rcritten commented Mar 28, 2023

flo-renaud commented Mar 29, 2023

flo-renaud left a comment

Choose a reason for hiding this comment

flo-renaud Mar 29, 2023

Choose a reason for hiding this comment

flo-renaud commented Mar 30, 2023

flo-renaud left a comment

Choose a reason for hiding this comment

flo-renaud commented Apr 4, 2023

mbhalodi commented Feb 28, 2023 •

edited