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

Cap pytest to below 3.0 #4826

Merged
merged 3 commits into from
Sep 19, 2016
Merged

Cap pytest to below 3.0 #4826

merged 3 commits into from
Sep 19, 2016

Conversation

ximenesuk
Copy link
Contributor

@ximenesuk ximenesuk commented Sep 8, 2016

What this PR does

This PR caps the pytest version to below 3.0.0 due to the bug at: pytest-dev/pytest#1905

Testing this PR

The test previously failing at https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-breaking-integration-Python27/26/testReport/OmeroPy.test.integration.clitest/test_download/TestDownload/ should pass.

When the issue above if resolved the commit in this PR should be reverted or otherwise amended.

See: https://trello.com/c/FjmLqCum/709-uncap-pytest

--rebased-to #4848

@mtbc
Copy link
Member

mtbc commented Sep 8, 2016

Maybe also need a card to help us remember to undo this once 3.0.3 is out.

@sbesson
Copy link
Member

sbesson commented Sep 8, 2016

Agreed. There might be a race condition between open-source project roadmaps and we should definitely watch the pytest releases. I would assume this PR could stay on hold for a few weeks and consider to be merged when we reach 5.3.0-m5 if nothing has been released upstream then. /cc @jburel

@ximenesuk
Copy link
Contributor Author

If the tests pass tomorrow then I assume the breaking tag will need to be removed to protect other builds?

@ximenesuk
Copy link
Contributor Author

@simleo
Copy link
Member

simleo commented Sep 9, 2016

To avoid coming back to this in the future we need to change the clause so that only bugged versions are excluded. The PR that fixes the bug has already been merged and is scheduled for inclusion in the 3.0.3 release. I've checked 3.0.{0,1,2} and they're all bugged, while 2.9.2 is OK. So if we change the clause to:

tests_require=['pytest != 3.0.0, != 3.0.1, != 3.0.2']

we should be OK now and in the (near) future. AFAIK, PEP440 does not allow to specify the above clause in a more compact way.

Note that this PR (with the above change) must not be reverted. Even though they're going to fix the bug, those three releases will stay bugged forever.

@ximenesuk
Copy link
Contributor Author

Thanks @simleo, I'll defer to @sbesson whether this PR needs a longer term solution or if it is just a temporary PR until pytest is fixed. I may not be here when it is!

@jburel jburel added the develop label Sep 9, 2016
@sbesson
Copy link
Member

sbesson commented Sep 9, 2016

Unless there is some work in breaking which requires Python tests, I would propose to include it in the merge build to ensure Python tests are run consistently. I hope this is a temporary solution but I would assume we do not have to decide on the inclusion until the next development milestone (5.3.0-m5) /cc @jburel . @simleo: agreed, the only compacter way I can think of would be pytest >= 3.0.3 once the latter is release (not necessary but sufficient condition).

@jburel
Copy link
Member

jburel commented Sep 9, 2016

Let's move it out of breaking. It does not hurt to keep this PR opened until the new version of pytest is release

@jburel
Copy link
Member

jburel commented Sep 9, 2016

Removed --breaking

@simleo
Copy link
Member

simleo commented Sep 13, 2016

@sbesson the problem with pytest >= 3.0.3 is that it would also unnecessarily block 2.*, forcing an upgrade to the latest version in the short term.

@mtbc
Copy link
Member

mtbc commented Sep 16, 2016

Could we have another version of this on breaking so that build goes green again?

@ximenesuk
Copy link
Contributor Author

@sbesson Is there a way for snoopy to merge it into both breaking and develop?

@sbesson
Copy link
Member

sbesson commented Sep 16, 2016

@ximenesuk @mtbc: It's all part of the semantics of the breaking queue. Right now, the push job is using merge develop --no-ask --reset -D none -I breaking -E exclude --push develop/breaking/trigger --update-gitmodules i.e. include no PR except for the ones labelled or commented as breaking. It is possible to change this contract to merge develop --no-ask --reset -I breaking --push develop/breaking/trigger --update-gitmodules which would include all PRs AND the breaking ones. The only downside is that there might be conflicts to resolve. No strong opinion either way.

The alternative would be to implement @simleo's #4826 (comment) and merge this PR. Leaving the OMERO 5.3 team to decide on the most convenient way

@ximenesuk
Copy link
Contributor Author

Let's leave the merge system as it is! I'll modify the PR inline with @simleo's suggestion and if the tests are okay on Monday the PR can be merged.

@ximenesuk
Copy link
Contributor Author

Commit added to exclude broken versions of pytest rather than cap it. If the tests pass then this should be merged to allow breaking to also go green.

@jburel
Copy link
Member

jburel commented Sep 19, 2016

https://ci.openmicroscopy.org/job/OMERO-DEV-merge-integration-python/370/
is green
Merging this PR so breaking become green.
We can always revisit later on.

@jburel jburel merged commit 14a76ca into ome:develop Sep 19, 2016
@atarkowska
Copy link
Member

--rebased-to #4858

@atarkowska
Copy link
Member

atarkowska commented Oct 10, 2016

I am afraid they haven;t fixed that in 3.0.3 as mentioned in #4826 (comment).

05:24:34 ============================= test session starts ==============================
05:24:34 platform linux2 -- Python 2.7.5, pytest-3.0.3, py-1.4.31, pluggy-0.4.0 -- /opt/hudson/workspace/OMERO-

I will open a new PR with

tests_require = ['pytest<3']

@jburel jburel added this to the 5.3.0 milestone Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants