search

sclorg / s2i-ruby-container

Ruby container images based on Red Hat Software Collections and intended for OpenShift and general usage, that provide a platform for building and running Ruby applications. Users can choose between Red Hat Enterprise Linux, Fedora, and CentOS based images.
http://softwarecollections.org
Apache License 2.0
55 stars 156 forks source link

Fix tests on fedora #556

Closed SlouchyButton closed 3 months ago

SlouchyButton commented 3 months ago

Two issues are addressed, both mentioned in https://github.com/sclorg/s2i-ruby-container/issues/554. When build fails, it will now handle the situation correctly and stop all depending tests from running while also causing the test to fail and return non-zero code. Code that is handling this is taken from s2i-python-container.

While debugging the code, I have encountered a duplicate function defined in the run file, despite already being present in test-lib.sh, so the local version was removed.

Dependency nio4r in puma-test-app is bumped to the newest version to prevent build failing. This is related to https://github.com/sclorg/s2i-ruby-container/issues/550.

Fixes: #554

phracek commented 3 months ago

@jackorp PTAL as well.

phracek commented 3 months ago

[test-all]

jackorp commented 3 months ago

LGTM.

I'd also like to raise one more thought. While the build failed, the tests didn't fail. Even though I'd expect the tests to be using the built container, IOW to fail at some point because they can't use the container, they used I presume previous build result. Would it make sense to clean up the container after the collections of test that need it finish?

SlouchyButton commented 3 months ago

LGTM.

I'd also like to raise one more thought. While the build failed, the tests didn't fail. Even though I'd expect the tests to be using the built container, IOW to fail at some point because they can't use the container, they used I presume previous build result. Would it make sense to clean up the container after the collections of test that need it finish?

I have also thought about that, and it seemed really weird, why can the test pass using a container that failed to build. It showed the same behavior even locally, even tho I have previously run tests on this container locally, so it would make sense that it used previous results from the last passed container build.

I can investigate this further, but I feel like it might be better to split it into different issue+PR not to make this too big, since I think that the major change here is the dependency update, not general test fixes - but that's up to you.

jackorp commented 3 months ago

it might be better to split it into different issue+PR

Absolutely. I was trying to find whether it was just me or if there is a cause for investigation.

I'll make an issue.

I think that the major change here is the dependency update

Agreed. More general test fixes can be left for other time.

SlouchyButton commented 3 months ago

it might be better to split it into different issue+PR

Absolutely. I was trying to find whether it was just me or if there is a cause for investigation.

I'll make an issue.

I think that the major change here is the dependency update

Agreed. More general test fixes can be left for other time.

Great, u can assign me to the issue, since I have already poked around the issue. I'll take a look at it when I'll have some spare time.

jackorp commented 3 months ago

@SlouchyButton #557 now exists, but I don't have sufficient perms to edit those fields.

phracek commented 3 months ago

[test-all]

phracek commented 3 months ago

I will fix OpenShift 4 tests later on. Let's get merge it. The failed tests do not block this PR.