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

Unbreak various tests in CI #3371

Merged
merged 4 commits into from
Jul 9, 2019
Merged

Conversation

Zlopez
Copy link
Contributor

@Zlopez Zlopez commented Jul 3, 2019

This should fix the failing CI tests

Signed-off-by: Michal Konečný mkonecny@redhat.com

@Zlopez Zlopez requested a review from a team as a code owner July 3, 2019 08:25
@Zlopez Zlopez added the Tests Issues pertaining to Bodhi's tests label Jul 3, 2019
@nphilipp
Copy link
Member

nphilipp commented Jul 4, 2019

I've updated the PR:

  • Added a description to the commit log about what went wrong here, i.e. backwards-incompatible changes in pytest 5.0 triggered the test failures. Because 5.0 dropped support for Python 2.x, we don't have it in Fedora yet, so the problem only showed up in some setups (Python modules installed with pip and up-to-date).
  • Renamed the context manager object from e to excinfo to make it more explicit we're not dealing with an exception here.
  • Rather than str(e.value), used the ExceptionInfo.exconly()` method which exposes name and message of the wrapped exception (and do this consistently in another test with the second commit).

@pypingou
Copy link
Member

pypingou commented Jul 4, 2019

Ok, I'm confused the test results show as red here but visiting: https://ci.centos.org/job/bodhi-pipeline/job/PR-3371/3/ I see Test Result (no failures)

is it still running?

@nphilipp
Copy link
Member

nphilipp commented Jul 4, 2019

Ok, I'm confused the test results show as red here but visiting: https://ci.centos.org/job/bodhi-pipeline/job/PR-3371/3/ I see Test Result (no failures)

is it still running?

Right now it still shows 6 as "expected". While the tests are in flight, I always look into the Pipeline Steps and, of the ones marked red, check their log (terminal symbol beside the red bubble). Basically, we have one issue in the pip-integration/-build tests with an unsigned package, and the sphinx problem in rawhide-docs.

@nphilipp nphilipp force-pushed the fix_skopeo branch 2 times, most recently from ae09f02 to d935451 Compare July 5, 2019 12:01
Copy link
Member

@pypingou pypingou left a comment

Choose a reason for hiding this comment

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

nice work tracking this down! :)

This is an obscure and hard to debug (therefore unfixed) problem in DNF
which caused installing Fedora Infrastructure RPM packages to fail in CI
containters. It's documented at:

https://bugzilla.redhat.com/show_bug.cgi?id=1699396

Thanks to Brian Stinson for the suggested workaround!

Signed-off-by: Nils Philippsen <nils@redhat.com>
@nphilipp
Copy link
Member

nphilipp commented Jul 8, 2019

With #3373 in flight I have had some time to think about str(excinfo.value) vs. excinfo.exconly(), especially that the latter isn't the same as the former but also includes the exception name. @Zlopez do you have an opinion?

@Zlopez
Copy link
Contributor Author

Zlopez commented Jul 8, 2019

@nphilipp Will excinfo.exconly() work with current tests?

@pypingou
Copy link
Member

pypingou commented Jul 8, 2019

Will excinfo.exconly() work with current tests?

Yes, we checked that, it was added in 2007 (iirc)

Prior to pytest 5.0, ExceptionInfo objects had a __str__() method of
their own which rendered the wrapped exception as a string. It was
removed in version 5.0 which made calling str() on the object use
__repr__() as a fallback which gives totally different results.

Here we use str() on the .value member (i.e. the wrapped exception) and
a variable name that makes it (more) explicit that the context manager
isn't an exception.

Signed-off-by: Michal Konečný <mkonecny@redhat.com>
Signed-off-by: Nils Philippsen <nils@redhat.com>
@nphilipp
Copy link
Member

nphilipp commented Jul 8, 2019

As discussed, let's broaden the scope of this PR and include:

@nphilipp nphilipp changed the title Fix skopeo tests Unbreak various tests in CI Jul 8, 2019
@nphilipp nphilipp added the reliability Issues pertaining to Bodhi's reliability label Jul 8, 2019
@nphilipp nphilipp added the WIP Work in progress label Jul 8, 2019
@nphilipp
Copy link
Member

nphilipp commented Jul 8, 2019

Oh, and as discussed I've replaced excinfo.exconly() with str(excinfo.value) as it's closer to what was done originally.

RUN rpm --import "$(awk -F= '{if ($0 ~ "gpgkey *=") {print $2;}}' < /etc/yum.repos.d/infra-tags.repo)"
# deal with https://fedoraproject.org/wiki/Changes/Set_skip_if_unavailable_default_to_false
RUN dnf install -y dnf-plugins-core
RUN dnf config-manager infrastructure-tags --setopt skip_if_unavailable=True --save
Copy link
Member

Choose a reason for hiding this comment

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

Every command adds a layer in the container, do you know if Randy and Aurélien preferred the one command / line approach? Otherwise, it is often recommended, to have multiple command per "RUN" using \ to delimit them.

Copy link
Member

@nphilipp nphilipp Jul 8, 2019

Choose a reason for hiding this comment

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

I don't know their preference. In the Dockerfiles there are instances of one RUN line containing multiple commands (though not many), but also multiple consecutive RUN lines each containing a single command.

@nphilipp
Copy link
Member

nphilipp commented Jul 8, 2019

For reference, here's an example of pydocstyle tripping: https://ci.centos.org/job/bodhi-pipeline/job/PR-3371/6/execution/node/329/log/

In previous versions, skip_if_unavailable defaulted to True so the
missing infrastructure-tags repository for the Rawhide release version
didn't make tests fail. Restore this setting for the infrastructure-tags
repository.

Signed-off-by: Nils Philippsen <nils@redhat.com>
@nphilipp nphilipp removed the WIP Work in progress label Jul 8, 2019
@Zlopez
Copy link
Contributor Author

Zlopez commented Jul 8, 2019

So only thing failing now will be rawhide docs test? Or this is a new issue?

@nphilipp
Copy link
Member

nphilipp commented Jul 8, 2019

So only thing failing now will be rawhide docs test? Or this is a new issue?

No, it's that cornice_sphinx is incompatible with the latest sphinx version: Cornices/cornice.ext.sphinx#22

@Zlopez
Copy link
Contributor Author

Zlopez commented Jul 8, 2019

I see that there are plenty of integration tests failing, but this will be probably related to #3370

This is because pydocstyle 4.0.0 can't cope with arguments described in
the docstrings of nested functions, as in bodhi/server/renderers.py:

PyCQA/pydocstyle#370

Signed-off-by: Nils Philippsen <nils@redhat.com>
@nphilipp
Copy link
Member

nphilipp commented Jul 9, 2019

As discussed: considering that we verified the changes in this PR to fix what they're supposed to do and that they aren't causing the remaining and new CI pipeline failures, I'll attempt to merge them manually, which should make fixing the outstanding issues easier as you don't have to dig through the errors this one is fixing.

@pypingou pypingou merged commit d5036bf into fedora-infra:develop Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reliability Issues pertaining to Bodhi's reliability Tests Issues pertaining to Bodhi's tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants