ipatests: nested netgroups (intg) #409

celestian · 2017-01-23T18:08:14Z

Adds a test case for issue in SSSD that manifested in
an inability to resolve nested membership in netgroups

The test case tests for direct and indirect membership.

https://fedorahosted.org/freeipa/ticket/6439

celestian · 2017-01-24T06:42:01Z

I see there are some issues:

It seems Travis hit some kind of time out. Maybe it is not connected to my proposed test.
The QuantifiedCode issues are clear.

I have PTO now and I will be back in work on Friday. But I hope I find a time to fix it sooner.

MartinBasti · 2017-01-24T07:33:36Z

Travis:

PEP-8 errors:
./ipatests/test_integration/test_netgroup.py:87:80: E501 line too long (84 > 79 characters)
./ipatests/test_integration/test_netgroup.py:96:80: E501 line too long (88 > 79 characters)


Pylint:
Pylint is running, please wait ...
************* Module ipatests.test_integration.test_netgroup
ipatests/test_integration/test_netgroup.py:2: [W0512(invalid-encoded-data), ] Cannot decode using encoding "ascii", unexpected byte at position 9)
ipatests/test_integration/test_netgroup.py:87: [E1101(no-member), TestNetgroups.check_users_in_netgroups] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:96: [E1101(no-member), TestNetgroups.check_nested_netgroup_hierarchy] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:125: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:130: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:131: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:132: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:147: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:148: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:149: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)
make: *** [pylint] Error 6

I'm afraid that pylint cannot handle it, so probably you have to disable problematic lines.

MartinBasti

I had a few comments. I think that relations and test data should be defined in a python structure and then used instead of doing iterations over range() call

MartinBasti · 2017-01-24T07:35:54Z

ipatests/test_integration/test_netgroup.py

@@ -0,0 +1,149 @@
+# Authors:
+#   Petr Čech <pcech@redhat.com>


Please use the short form of the header, you can find it in test_dnssec.py

MartinBasti · 2017-01-24T07:38:00Z

ipatests/test_integration/test_netgroup.py

+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+from __future__ import print_function


Please remove this import, print is not used in code

MartinBasti · 2017-01-24T07:53:58Z

ipatests/test_integration/test_netgroup.py

+
+@pytest.fixture()
+def three_netgroups(request):
+    for i in range(1, 4):


I'd like to have dict of names instead range(1, 4) all the time

test_data = [ { 'user': { 'login': 'testuser1', 'first': 'user1' }, 'netgroup': 'testgroup1', } } ] # this dict can be generated, you don't have to write it manually

and then

for d in test_data: # user-add d['user']['login'] --first=d['user'][first] ... # netgroup-add d['netgroup'] # netgroup-add-member d['netgroup'] --users=d['user']['name']

MartinBasti · 2017-01-24T08:03:22Z

ipatests/test_integration/test_netgroup.py

+                                        '--users=testuser%d' % i,
+                                        'test_netgroup%d' % i])
+
+    def teardown_three_netgroups():


use test_data from previous comment too here please

and it can be in one cycle

MartinBasti · 2017-01-24T08:08:18Z

ipatests/test_integration/test_netgroup.py

+
+    def teardown_three_netgroups():
+        for i in range(1, 4):
+            request.cls.master.run_command(['ipa', 'netgroup-del',


You need to prevent failures in teardown method.

Please use run_command(['*'], raiseonerr=False) it will not raise an exception when failure occurs

MartinBasti · 2017-01-24T08:14:24Z

ipatests/test_integration/test_netgroup.py

+        super(TestNetgroups, cls).install(mh)
+
+        cls.client = cls.clients[0]
+        cls.clientname = cls.client.run_command(


Why do you need a clientname AFAIK client should contain hostname as attribute.

MartinBasti · 2017-01-24T08:19:46Z

ipatests/test_integration/test_netgroup.py

+        cls.clientname = cls.client.run_command(
+            ['hostname', '-s']).stdout_text.strip()
+
+        cls.domain = cls.get_domains()[0]


IMHO cls.domain should already contain the domain

If you remove all cls. definitions in install method, you can remove it

MartinBasti · 2017-01-24T08:21:30Z

ipatests/test_integration/test_netgroup.py

+    def install(cls, mh):
+        super(TestNetgroups, cls).install(mh)
+
+        cls.client = cls.clients[0]


Could you rather use

client = self.clients[0]

directly in tests? It is more consistent with other tests

MartinBasti · 2017-01-24T08:31:06Z

ipatests/test_integration/test_netgroup.py

+    def check_users_in_netgroups(self):
+        clear_sssd_cache(self.client)
+        for i in range(1, 4):
+            result = self.client.run_command(['getent', 'passwd',


you have to add raiseonerr=False to receive result, otherwise exception will be raised, or you can remove assert result.returncode == 0

MartinBasti · 2017-01-24T08:38:11Z

ipatests/test_integration/test_netgroup.py

+
+    def check_nested_netgroup_hierarchy(self):
+        clear_sssd_cache(self.client)
+        for i in range(1, 4):


IMO nesting hierarchy can be represent similarly as test_data

celestian · 2017-02-02T15:32:21Z

I addressed all comment, I hope.
I know that there are still some pylint stuff like:

ipatests/test_integration/test_netgroup.py:91: [E1101(no-member), TestNetgroups.check_users_in_netgroups] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:108: [E1101(no-member), TestNetgroups.check_nested_netgroup_hierarchy] Class 'domain' has no 'name' member)
ipatests/test_integration/test_netgroup.py:132: [E1101(no-member), TestNetgroups.test_remove_nested_netgroup] Class 'domain' has no 'name' member)

I don't know why due to line 228 and 230 in pylint_plugins.py. Missed I something?

celestian · 2017-02-03T11:50:23Z

I addressed issue in Travis and Quantified.
New version pushed.

PS: Right way is self.master.domain.name

MartinBasti · 2017-02-03T12:45:15Z

ipatests/test_integration/test_netgroup.py

@@ -0,0 +1,183 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license


2017 please

celestian · 2017-02-03T12:52:34Z

2015-->2017 addressed. New version pushed.

MartinBasti · 2017-02-03T13:25:21Z

ipatests/test_integration/test_netgroup.py

+        }
+        test_data.append(data)
+
+        members = [d['user']['login'] for d in test_data] if test_data else []


You dont need, if test_data else [] it behaves the same when test_data == []

MartinBasti · 2017-02-03T13:26:17Z

ipatests/test_integration/test_netgroup.py

+                'last': 'User_{}'.format(i),
+            },
+            'netgroup': 'testgroup_{}'.format(i),
+            'nested_netgroup': 'testgroup_{}'.format(i-1) if i > 1 else ''


I prefer None for nothing, instead of empty string

MartinBasti · 2017-02-03T13:28:11Z

ipatests/test_integration/test_netgroup.py

+from ipatests.test_integration.tasks import clear_sssd_cache
+
+
+def get_test_data():


Could be test_data a module variable instead of function, I don't see reason to generate the same data again and again

MartinBasti · 2017-02-03T13:43:38Z

ipatests/test_integration/test_netgroup.py

+    Test Netgroups
+    """
+
+    num_clients = 1


Can be this tested on server side? Do we need to install extra machine?

celestian · 2017-02-03T16:23:34Z

Addressed, pushed.
Thanks for useful comments :-)

rcritten · 2017-02-06T13:25:45Z

Contributors.txt

@@ -98,6 +98,7 @@ Developers:
 	Adam Young
 	Jan Zelený
 	Pavel Zůna
+	Petr Čech


nit: this file is sorted alphabetically

Addressed, thanks.

Adds a test case for issue in SSSD that manifested in an inability to resolve nested membership in netgroups The test case tests for direct and indirect membership. https://fedorahosted.org/freeipa/ticket/6439

MartinBasti · 2017-02-07T10:56:33Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/dc99d3c04e43b08d2364209a641b8b9111e5986c

MartinBasti self-assigned this Jan 24, 2017

martbab mentioned this pull request Jan 24, 2017

ipatests: unresolvable nested netgroups #196

Closed

MartinBasti suggested changes Jan 24, 2017

View reviewed changes

celestian force-pushed the test_nested_netgroup branch from aa0707a to 756a7e0 Compare February 2, 2017 15:29

celestian force-pushed the test_nested_netgroup branch from 756a7e0 to 1398edd Compare February 3, 2017 11:46

MartinBasti reviewed Feb 3, 2017

View reviewed changes

celestian force-pushed the test_nested_netgroup branch from 1398edd to 0b5c152 Compare February 3, 2017 12:51

MartinBasti suggested changes Feb 3, 2017

View reviewed changes

celestian force-pushed the test_nested_netgroup branch from 0b5c152 to 998fe25 Compare February 3, 2017 16:23

rcritten reviewed Feb 6, 2017

View reviewed changes

ipatests: nested netgroups (intg)

580daa2

Adds a test case for issue in SSSD that manifested in an inability to resolve nested membership in netgroups The test case tests for direct and indirect membership. https://fedorahosted.org/freeipa/ticket/6439

celestian force-pushed the test_nested_netgroup branch from 998fe25 to 580daa2 Compare February 6, 2017 13:37

MartinBasti approved these changes Feb 6, 2017

View reviewed changes

MartinBasti added the ack Pull Request approved, can be merged label Feb 6, 2017

MartinBasti added the pushed Pull Request has already been pushed label Feb 7, 2017

MartinBasti closed this Feb 7, 2017

ipatests: nested netgroups (intg) #409

ipatests: nested netgroups (intg) #409

celestian commented Jan 23, 2017 •

edited by MartinBasti

celestian commented Jan 24, 2017

MartinBasti commented Jan 24, 2017

MartinBasti left a comment

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

MartinBasti Jan 24, 2017

celestian commented Feb 2, 2017

celestian commented Feb 3, 2017

MartinBasti Feb 3, 2017

celestian commented Feb 3, 2017

MartinBasti Feb 3, 2017

MartinBasti Feb 3, 2017

MartinBasti Feb 3, 2017

MartinBasti Feb 3, 2017

celestian commented Feb 3, 2017 •

edited

rcritten Feb 6, 2017

celestian Feb 6, 2017

MartinBasti commented Feb 7, 2017

		@@ -0,0 +1,183 @@
		#
		# Copyright (C) 2015 FreeIPA Contributors see COPYING for license

		from ipatests.test_integration.tasks import clear_sssd_cache


		def get_test_data():

ipatests: nested netgroups (intg) #409

ipatests: nested netgroups (intg) #409

Conversation

celestian commented Jan 23, 2017 • edited by MartinBasti

celestian commented Jan 24, 2017

MartinBasti commented Jan 24, 2017

MartinBasti left a 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

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

celestian commented Feb 2, 2017

celestian commented Feb 3, 2017

Choose a reason for hiding this comment

celestian commented Feb 3, 2017

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

celestian commented Feb 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Feb 7, 2017

celestian commented Jan 23, 2017 •

edited by MartinBasti

celestian commented Feb 3, 2017 •

edited