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

[MRG] Make the memory limit test simpler #912

Merged
merged 2 commits into from Jun 16, 2020

Conversation

betatim
Copy link
Member

@betatim betatim commented Jun 12, 2020

Instead of checking that a build which allocates too much memory does
fail, we now only check that we pass the correct arguments to the Docker
API client. It seems reasonable to rely on the docker client working and
doing the right thing. This solves a problem where our CI is flakey
because the kernel of the VM the tests run on doesn't support this.

Hopefully this will make our CI faster and less flakey.

Instead of checking that a build which allocates too much memory does
fail, we now only check that we pass the correct arguments to the Docker
API client. It seems reasonable to rely on the docker client working and
doing the right thing. This solves a problem where our CI is flakey
because the kernel of the VM the tests run on doesn't support this.
@betatim betatim requested a review from yuvipanda June 12, 2020 17:05
@manics
Copy link
Member

manics commented Jun 12, 2020

Look like the new release of pytest-cov which requires pytest>=4.6 broke travis:
https://github.com/pytest-dev/pytest-cov/blob/master/CHANGELOG.rst#2100-2020-06-12
Though I can't see why, https://github.com/jupyter/repo2docker/blob/8d85a51cd2b8d7dee712d3c1b283de1af734396d/dev-requirements.txt#L4 only has a lower bound, not an upper bound.

@betatim
Copy link
Member Author

betatim commented Jun 12, 2020

I think we got pytest==3.6 because that got installed previously and was in the build cache. Cleared the caches and restarting the build.

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

My name is Yuvi Panda and I approve this PR

@betatim
Copy link
Member Author

betatim commented Jun 13, 2020

The mystery continues, the pytest-cov setup.py contains this:

https://github.com/pytest-dev/pytest-cov/blob/694f7fd497b07aaea76d96e484068faf440e8301/setup.py#L124-L127

so why doesn't pip install a newer version of pytest? It even realises that there is a conflict and tells us "ERROR: pytest-cov 2.10.0 has requirement pytest>=4.6, but you'll have pytest 4.3.1 which is incompatible.'

Seems like pypa/pip#6969 might be the answer to explain why this is happening.

@betatim
Copy link
Member Author

betatim commented Jun 13, 2020

Pushed a new commit that removes the minimum version requirement for pytest that we have in dev-requirements.txt. If I understand the pip issue linked in the previous comment then this might lead to us getting a version of pytest that is driven by pytest-cov's requirement.

Pushed a new commit that bumps up our minimum pytest version to what pytest-cov needs. Maybe we could also solve this by listing pytest-cov before pytest in our dev-requirements.txt but it seems not worth the time to experiment as explicitly listing the pytest version seems more explicit :)

Side comment: what do I do with the Pipfile.lock? How does that get updated? It also makes me wonder why we have that file checked in. repo2docker is a library, so we don't need to exactly reproduce the installed versions from my laptop to all other dev's machines. Anything within the limits of our dev-requirements.txt should work and if not that is a bug. So maybe we shouldn't checkin the lock file? (If we were generating an env for a production system then I'd want a lockfile so that rebuilding that env across time is stable but I don't think we have the requirement for a dev environment.)

pytest-cov requires pytest>=4.6 so we explicitly list that as a dev
dependency.
@betatim
Copy link
Member Author

betatim commented Jun 13, 2020

The robots are happy 🎉! The codecov test fails but I think that is expected. Looking at https://codecov.io/gh/jupyter/repo2docker/compare/8d85a51cd2b8d7dee712d3c1b283de1af734396d...d135e3fbd1675912f105151be00334c368d693af/changes I think we'd need to add a test that fails the build :-/ New issue?

@betatim
Copy link
Member Author

betatim commented Jun 16, 2020

There is one approval and the bots are happy so merging this.

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