Add container PyTest to Ruby Container #608
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request validationFailed🔴 Review - Missing review from a member (1 required) Success🟢 CI - All checks have passed |
phracek
force-pushed
the
container_pytests
branch
from
c359e76 to
21776e9
Compare
Member
Author
|
[test-pytest][test-openshift-pytest] |
Testing Farm results
|
phracek
force-pushed
the
container_pytests
branch
from
e68a3cc to
6a40ee4
Compare
phracek
force-pushed
the
container_pytests
branch
from
b24e784 to
69acf3b
Compare
Member
Author
|
[test-pytest][test-openshift-pytest] |
Member
Author
|
[test-all] |
Signed-off-by: Petr "Stone" Hracek <[email protected]>
Migration matrix is following: For db, puma, rack applications are classes: db -> TestRubyHelloWorldContainer puma -> TestRubyPumaTestAppContainer rack -> TestRubyRackTestAppContainer test_docker_run_usage -> test_container_basics.py->TestS2IRubyContainer(test_docker_run_usage) test_application -> test_application in each class test_connection -> test_application in each class in assert test_response -> test_application in each class in assert test_scl_usage -> test_container_basics.py->TestS2IRubyContainer(test_scl_usage) test_npm_functionality -> test_container_basics.py->TestRubyNPMtestContainer test_ruby_fips_mode -> test_container_fips.py->TestRubyFipsModeContainer(test_fips_mode) test_ruby_fips_s2i_app -> test_container_fips.TestRubyFipsApplicationContainer(test_application) test_from_dockerfile -> test_container_basics.py -> TestS2IRubyContainer(test_dockerfiles) test_from_dockerfile.s2i -> test_container_basics.py -> TestS2IRubyContainer(test_dockerfiles) Signed-off-by: Petr "Stone" Hracek <[email protected]>
Do not run FIPS tests on RHEL8 Signed-off-by: Petr "Stone" Hracek <[email protected]>
Fix skipping RHEL8 tests for FIPS. Signed-off-by: Petr "Stone" Hracek <[email protected]>
Build app once and test it. Signed-off-by: Petr "Stone" Hracek <[email protected]>
Signed-off-by: Petr "Stone" Hracek <[email protected]>
Signed-off-by: Petr "Stone" Hracek <[email protected]>
phracek
force-pushed
the
container_pytests
branch
from
9f63909 to
c8f48c2
Compare
Member
Author
|
Rebased agains master. [test-all] |
jackorp
requested changes
Jan 5, 2026
Contributor
jackorp
left a comment
jackorp
left a comment
There was a problem hiding this comment.
I cannot promise that this review is by any means exhaustive to the last letter of the changes.
Bar the incorrect docstrings, there are some opportunities to refactor, PTAL.
Comment on lines
+53
to
+55
| BRANCH_TO_TEST = "master" | ||
| if VERSION == "3.1" or VERSION == "3.3": | ||
| BRANCH_TO_TEST = "3.3" |
Contributor
There was a problem hiding this comment.
This seems as kind of very general name. How about:
TEST_APP_BRANCH = "master"
if VERSION == "3.1" or VERSION == "3.3":
TEST_APP_BRANCH = "3.3"Also since we are going to some language that's not stringly typed, how about:
Suggested change
| BRANCH_TO_TEST = "master" | |
| if VERSION == "3.1" or VERSION == "3.3": | |
| BRANCH_TO_TEST = "3.3" | |
| TEST_APP_BRANCH = "master" | |
| if VERSION and float(VERSION) >= 3.1 : | |
| TEST_APP_BRANCH = "3.3" |
Unless python has some support for versions (in Ruby, we could use e.g. Gem::Version class for semantically proper version comparisons)
In this case, only retyping the VERSION here so far, first file I am seeing now.
| #!/bin/bash | ||
| # | ||
| # IMAGE_NAME specifies a name of the candidate image used for testing. | ||
| # The image has to be available before this script is executed. |
Contributor
There was a problem hiding this comment.
could the existence of the image be checked from the script and fail with proper error message if it is not available?
Should it fail if IMAGE_NAME is not available?
Comment on lines
+51
to
+66
| if VARS.OS == "rhel8": | ||
| pytest.skip("Do not execute on RHEL8") | ||
| if fips_enabled(): | ||
| output = PodmanCLIWrapper.podman_run_command_and_remove( | ||
| cid_file_name={VARS.IMAGE_NAME}, | ||
| cmd="ruby -ropenssl -e 'exit OpenSSL.fips_mode'", | ||
| ) | ||
| print(f"FIPS is enabled {output}") | ||
| assert output | ||
| else: | ||
| output = PodmanCLIWrapper.podman_run_command_and_remove( | ||
| cid_file_name=f"{VARS.IMAGE_NAME}-{self.app.app_name}", | ||
| cmd="ruby -ropenssl -e 'exit !OpenSSL.fips_mode'", | ||
| ) | ||
| print(f"FIPS is disable {output}") | ||
| assert not output |
Contributor
There was a problem hiding this comment.
I assume this works correctly, but I am not sure what is being asserted.
So, there are 3 possibilities based on how well I read PodmanCLIWrapper.podman_run_command_and_remove
- returncode is returned, what is outlined here works because returncode is returned.
- STDOUT
&&/||STDERR output is present, what is outlined here does not work and will blow up the moment our assumptions were wrong. (for example, we get a false positive/negative) - I read function stack incorrectly and it all just works.
test/run-pytest
Outdated
| # | ||
| # IMAGE_NAME specifies a name of the candidate image used for testing. | ||
| # The image has to be available before this script is executed. | ||
| # SINGLE_VERSION specifies the major version of the MariaDB in format of X.Y |
Contributor
There was a problem hiding this comment.
Suggested change
| # SINGLE_VERSION specifies the major version of the MariaDB in format of X.Y | |
| # SINGLE_VERSION Specifies the image version - (must match with subdirectory in repo) |
comment copy from common/build.sh
Comment on lines
+75
to
+78
| """ | ||
| Test if building nginx-container based on | ||
| examples/Dockerfile works | ||
| """ |
Contributor
There was a problem hiding this comment.
Suggested change
| """ | |
| Test if building nginx-container based on | |
| examples/Dockerfile works | |
| """ | |
| Test if building apps based on Containerfiles in | |
| examples/ works |
| """ | ||
| self.oc_api.delete_project() | ||
|
|
||
| def test_dancer_ex_template_inside_cluster(self): |
Contributor
There was a problem hiding this comment.
dancer_ex? I assume that's incorrect
Suggested change
| def test_dancer_ex_template_inside_cluster(self): | |
| def test_rails_ex_template_inside_cluster(self): |
| from conftest import VARS | ||
|
|
||
|
|
||
| DEPLOYED_PGSQL_IMAGE = "quay.io/sclorg/postgresql-12-c8s" |
Contributor
There was a problem hiding this comment.
Maybe move to conftest.py? @phracek
I think the cases here are that the rails ex either has a template which specifies the image, or we are looking for a specific image.
Either way, the app either depends on specific PGSQL of version XYZ, or we will want to move it to later releases/distros. Using conftest as a single place might be advantageous.
| from conftest import VARS | ||
|
|
||
|
|
||
| DEPLOYED_PGSQL_IMAGE = "quay.io/sclorg/postgresql-12-c8s" |
Contributor
There was a problem hiding this comment.
same comment as before, but for completeness, maybe move to conftest.py.
| @@ -0,0 +1,68 @@ | |||
| import pytest | |||
Contributor
There was a problem hiding this comment.
Maybe merge the file with test_ocp_s2i_local_templates.py and instead of differing in only 1 method (in how the template is sourced), add 1 test method that is the difference. More opportunities to refactoring to less code present themselves if the files are merged.
| from conftest import VARS | ||
|
|
||
|
|
||
| class TestHelmCakePHPTemplate: |
Contributor
There was a problem hiding this comment.
Suggested change
| class TestHelmCakePHPTemplate: | |
| class TestHelmRailsRubyTemplate: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add Container PyTest suite to s2i-ruby container.
Migration matrix is following:
For db, puma, rack applications are classes:
db -> TestRubyHelloWorldContainer
puma -> TestRubyPumaTestAppContainer
rack -> TestRubyRackTestAppContainer