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

Fix mocking insufficiencies in system.network.py #2717 #2718

Conversation

FroggyFlox
Copy link
Member

Fixes #2717
@phillxnet, @Hooverdan96: ready for review

As detailed in #2717, we have two mocking deficiencies:

  • one that resulted in one test actually writing a file to disk
  • one failing to properly mock os.scandir() (we were thus depending on the real system state)

This pull request fixes the former by combining 2 previously split tests, thereby sharing the same mock (we're not writing to disk anymore). To fix the latter, we instantiate a new class used to mock os.scandir return values that we can now properly control.

To test, Tailscale was entirely removed from the system and the repo removed as well. All tests now pass and the absence of /etc/sysctl.d/99-tailscale.conf (which was erroneously making test_disable_ip_forwarding() pass before) was verified.

buildvm155:/opt/rockstor/src/rockstor # export DJANGO_SETTINGS_MODULE=settings
buildvm155:/opt/rockstor/src/rockstor # poetry run django-admin test
Creating test database for alias 'default'...
Creating test database for alias 'smart_manager'...
System check identified no issues (0 silenced).
......................................................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 278 tests in 15.243s

OK

Hopefully this really corrects the situation this time and not just on my machine.

We had two mocking deficiencies:
 - one that resulted in one test actually writing a file to disk
 - one failing to properly mock os.scandir (we were thus depending on
   the real system state)

This commit fixes the former by combining 2 previously split tests,
thereby sharing the same mock (we're not writing to disk anymore).
To fix the latter, we instantiate a new class used to mock os.scandir
return values that we can now properly control.
@FroggyFlox FroggyFlox linked an issue Oct 19, 2023 that may be closed by this pull request
Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

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

@FroggyFlox Thanks for getting to this fix so quickly.
By way of confirmation, I have, with git release 5.0.5 code confirmed that we have the failure when the file "/etc/sysctl.d/99-tailscale.conf" does not exist, but there after it does exist. And further test runs all pass.

Removing the file again and building an RPM but with this proposed changed included and all is well: in that the %check scriptlet runs all tests just fine and there-after there is still no /etc/sysctl.d/99-tailscale.conf file.

A further run of all tests via cli from the RPM installed code also succeeds and again without this file having been created.

So all good. Well done on the minimalist mock re MockDirEntry. There is a facility to mock an entire filesystem I believe - but never used it and can't remember what it was! Maybe with our recent Python updates we could look to this for some of our file interactions.

@phillxnet phillxnet merged commit ee4af7c into rockstor:testing Oct 20, 2023
@FroggyFlox
Copy link
Member Author

Thanks for the confirmation; I'm glad it holds up during the %check step!

Well done on the minimalist mock re MockDirEntry.

You've got the collective mind of the web to thank for that one.

There is a facility to mock an entire filesystem I believe - but never used it and can't remember what it was! Maybe with our recent Python updates we could look to this for some of our file interactions.

I've seen references to pyfakefs (https://pypi.org/project/pyfakefs/) when trying to figure these tests. Maybe that's the one you're thinking of? It seems to work with both pytest and unittest.

@phillxnet
Copy link
Member

phillxnet commented Oct 20, 2023

@FroggyFlox Re:

I've seen references to pyfakefs (https://pypi.org/project/pyfakefs/) when trying to figure these tests. Maybe that's the one you're thinking of? It seems to work with both pytest and unittest

Yes that sounds like it, not certain though. I heard a mention of such a thing on PythonBytes or TalkPython and Mr Python testing, Brian Okken, @[email protected] made mention.


OK just found at least one mention:
Apparently in the following episode:
https://pythonbytes.fm/episodes/show/277/its-a-python-package-showdown
Special guest: Thomas Gaigher makes mention.

But I expect Mr Okken has mentioned it on other occasions :) .

@FroggyFlox FroggyFlox deleted the 2717-Initial-rpmbuild-failure---test_disable_ip_forwarding branch October 20, 2023 20:30
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.

Initial rpmbuld failure - test_disable_ip_forwarding
2 participants