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

Possible patching bug/maintainability concerns in test_util.py #16

Open
ChrisKeefe opened this issue May 29, 2020 · 0 comments
Open

Possible patching bug/maintainability concerns in test_util.py #16

ChrisKeefe opened this issue May 29, 2020 · 0 comments

Comments

@ChrisKeefe
Copy link
Collaborator

ChrisKeefe commented May 29, 2020

Improvement Description
Tests in test_util.py which use mock.patch are wordy, and do not line up nicely with samples from the python docs. Further, they exhibit unusual behavior I haven't yet been able to explain, which raises concerns that they may not be functioning as intended

Current Behavior

    @mock.patch("q2_diversity_lib._util.psutil.Process")
    def test_function_with_an_n_jobs_param(self, mock_process):
        mock_process = psutil.Process()
        mock_process.cpu_affinity = mock.MagicMock(return_value=[0, 1, 2])
        self.assertEqual(self.function_w_n_jobs_param(3), 3)

If we remove mock_process = psutil.Process() from the above snippet, the tests fails, and calls to psutil.Process.cpu_affinity() in the code under test return a mock object instead of returning the assigned return_value=[0, 1, 2]

Proposed Behavior
More succinct and idiomatic testing will improve confidence in test behavior.

Comments

  1. If possible, test_system_has_no_cpu_affinity() should continue to operate without using patch's create=true option. (This arg tells patch to create nonexistent attributes). The code is uglier, but the semantics are cleaner if we let tests run on macOS trigger create AttributeErrors "naturally" than if we patch in an artificial cpu_affinity method which yields a patched AttributeError.

References

  1. Discussion/docs link in this comment
  2. Attempts to remedy in this PR
  3. Further notes
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

No branches or pull requests

1 participant