rockstor / rockstor-core

Linux/BTRFS based Network Attached Storage(NAS)
http://rockstor.com/docs/contribute_section.html
GNU General Public License v3.0
552 stars 137 forks source link

Insufficient mocking in `test_user.py` #2647

Open FroggyFlox opened 1 year ago

FroggyFlox commented 1 year ago

Although we use fixtures to define users used during the test_user.py test suite, we fail to properly mock the system users in the same tests. As a result, if the system contains a user with the same name as one defined in the fixture, we encounter conflicts between the two, such as:

======================================================================
FAIL: test_post_requests (rockstor.storageadmin.tests.test_user.UserTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/storageadmin/tests/test_user.py", line 198, in test_post_requests
    msg=response.data,
AssertionError: 200 != 500 : {'id': 3, 'groupname': 'users', 'managed_user': True, 'has_pincard': False, 'pincard_allowed': 'no', 'username': 'newUser44', 'uid': 3, 'gid': 5, 'public_key': None, 'shell': '/bin/bash', 'homedir': '/home/newUser44', 'email': None, 'admin': True, 'user': 5, 'group': 1, 'smb_shares': []}

----------------------------------------------------------------------

This happens when the system already has a user named testuser. Given our fixture creates a user with the same name but with different attribute, it can lead to errors such as above. See relevant part of the fixture below: https://github.com/rockstor/rockstor-core/blob/387667dfd393d5c424fd1fa4892e61518cc3b75e/src/rockstor/storageadmin/fixtures/test_user.json#L22-L36

Reproducer

  1. In Rockstor webUI, create a new user:
    • name: testuser
    • UID: anything but 1004
    • NOT a Rockstor admin
  2. run tests: poetry run django-admin test -p test_user*
FroggyFlox commented 1 year ago

In this case, we fetch system users from: https://github.com/rockstor/rockstor-core/blob/387667dfd393d5c424fd1fa4892e61518cc3b75e/src/rockstor/storageadmin/views/user.py#L151-L152

It is thus proposed that we mock combined_users() in the test_user.py test suite.

Note that we likely will also need to mock combined_groups() as well.

FroggyFlox commented 5 months ago

Thanks a lot for remembering and linking to that issue from #2816, @phillxnet, I had completely forgotten. If my memory is correct on that one, what blocked me at the time was that combined_users() returns a queryset and I wasn't sure how to mock that properly.