RHOAIENG-17251: add a test for the proxy env configuration in RStudio + some other minor changes #870
Conversation
This is to follow the proper filename convention so that the pytest will be able to find the tests in this file accordingly.
|
lgtm, just here's what I'd do in addition i'm a big fan of table driven tests (https://dave.cheney.net/2019/05/07/prefer-table-driven-tests#Introducing-table-driven-tests), so @allure.issue("RHOAIENG-16604")
def test_http_proxy_env_propagates(self, subtests, image: str) -> None:
"""
This checks that the lowercased proxy configuration is propagated into the RStudio
environment so that the appropriate values are then accepted and followed.
"""
skip_if_not_rstudio_image(image)
class TestCase(NamedTuple):
name: str
name_lc: str
value: str
test_cases: list[TestCase] = [
TestCase("HTTP_PROXY", "http_proxy", "http://localhost:8080"),
TestCase("HTTPS_PROXY", "https_proxy", "https://localhost:8443"),
TestCase("NO_PROXY", "no_proxy", "google.com"),
]
container = WorkbenchContainer(image=image, user=1000, group_add=[0])
for tc in test_cases:
container.with_env(tc.name, tc.value)
try:
# We need to wait for the IDE to be completely loaded so that the envs are processed properly.
container.start(wait_for_readiness=True)
# Once the RStudio IDE is fully up and running, the processed envs should include also lowercased proxy configs.
for tc in test_cases:
with subtests.test(tc.name):
output = check_output(
container,
f"/usr/bin/R --quiet --no-echo -e 'Sys.getenv(\"{tc.name_lc}\")'")
assert '"' + tc.value + '"' in output
finally:
docker_utils.NotebookContainer(container).stop(timeout=0)one advantage of subtest is that when variable is missing, it will also check all the other variables before failing the test it also makes things simpler if there's additional variable to be checked, just add it to the test_cases table |
|
/lgtm |
|
@jiridanek thanks for your review - I updated based on your comment. Please, check again. |
| class TestCase(): | ||
| def __init__(self, name, name_lc, value): | ||
| self.name = name | ||
| self.name_lc = name_lc | ||
| self.value = value |
There was a problem hiding this comment.
I'd nest this under def test_, because this only applies for that one test and no other
and using NamedTuple, or @dataclass is better here, imo; avoids having to repeat the field names twice
- https://docs.python.org/3.12/tutorial/classes.html#odds-and-ends
- https://docs.python.org/3/library/typing.html#typing.NamedTuple
compared do my version, you dropped the :str type annotations ;p
Includes a simple test to check that the variables are propagated properly so that the proxy configuration can work in RStudio IDE as expected. * https://issues.redhat.com/browse/RHOAIENG-17251
Let's not use trailing slash for the directory. When this variable is used and some value should be append to it, it should be done always as: `APP_ROOT_HOME/some-path`. So the forward slash is added there and we avoid having double slash in our outputs that may be misleading (e.g. empty variable value in the path).
|
/lgtm |
|
Thank you for your review and tips, @jiridanek ! /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstourac The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/images |
|
@jstourac: Overrode contexts on behalf of jstourac: ci/prow/images In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Description
https://issues.redhat.com/browse/RHOAIENG-17251
How Has This Been Tested?
Merge criteria: