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 util.misc.available_cpu_count() #912

Merged
merged 8 commits into from Jan 30, 2019
Merged

fix util.misc.available_cpu_count() #912

merged 8 commits into from Jan 30, 2019

Conversation

notestaff
Copy link
Contributor

Fixes util.misc.available_cpu_count() . Also adds some generally useful pytest utils.

@notestaff notestaff self-assigned this Jan 18, 2019
@@ -83,6 +87,44 @@ def tmpdir_function(request, tmpdir_class, monkeypatch):
monkeypatch.setenv('TMPDIR', tmpdir)
yield tmpdir

@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be a fixture and contextmanager. The pytest philosophy doesn't want to use with mock(obj) as m... lines in the test cases because it breaks up the flow. https://pypi.org/project/pytest-mock/. For your actual test on the CPU counts the simple mock.return_value = '1' would suffice. You can perhaps monkeypatch the MockFixture module to add this functionality if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The available_cpu_count tests needs to monkeypatch different return values of slurp_file for different arguments. Also, a contextmanager is needed if you want to test more than one thing within one test function. monkeypatch with context managers is built into pytest, so pytest philosophy does allow for that.

Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

I'm not 100% following what's going on in the monkeypatch code, but I like all of the other changes in the rest of the PR and am interested in getting this merged in soon. I'm fine with introducing a new fixture that, so far, is only used in one place, though I'm not quite clear exactly which code paths are being tested by it. Wouldn't the new tests-docker.sh sufficiently test the available_cpu_count function, the main drawback being that it doesn't get credit for it in coveralls?

@notestaff
Copy link
Contributor Author

The situation in #911, where cfs_quota_us is reporting -1, doesn’t exist on travis, so the moneypatching simulates it.

@dpark01
Copy link
Member

dpark01 commented Jan 30, 2019

Hm, yes worth mocking all the possible values, I'm just not used to this way of mocking it. Let's just go with this.

I do see -1 from that cgroup file in the usual PAPIv2 setup, my guess is that test-docker.sh is actually getting -1 if --cpus was not passed to docker run (but then you wouldn't know what the expected return value of the function would be).

@notestaff notestaff merged commit ed106c3 into master Jan 30, 2019
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.

None yet

3 participants