Update 'identical hostname' tests since the behavior has changed #599
Conversation
...ests/singlecluster/identical_hostnames/auth/test_identical_hostnames_auth_on_gw_and_route.py
Outdated
Show resolved
Hide resolved
...ests/singlecluster/identical_hostnames/auth/test_identical_hostnames_auth_on_gw_and_route.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Tomas Repel <[email protected]>
trepel
force-pushed
the
identical-hostname-test-updates
branch
from
9ab2189 to
63291a5
Compare
|
@averevki Thanks for review! Your suggestions have been force pushed. Could you please take a look if you get a chance? |
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def rate_limit(): |
There was a problem hiding this comment.
Common practice across testsuite is creating new commit fixtures, with objects you want to create. In this case just authorization
There was a problem hiding this comment.
So in theory this can be removed I assume. I have a commit fixture defined in the test files directly and RLP is not committed there.
There was a problem hiding this comment.
It seems it indeed can be removed. Will force push and let you know.
There was a problem hiding this comment.
@jsmolar force-pushed now
|
|
||
|
|
||
| @pytest.fixture(scope="module", autouse=True) | ||
| def commit(request, authorization, authorization2): |
There was a problem hiding this comment.
This fixture should be in auth conftest. The same applies for commit in test_identical_hostnames_auth_on_routes.py
There was a problem hiding this comment.
This fixture is slightly different for each test. In 'on route' scenario both auth policies are fully enforced whereas in 'on gw' scenario the 2nd authpolicy (the one attached to GW rather than HTTPRoute) is overridden.
It did not occur to me how to handle this difference nicely in the fixture so that it can be used for both tests, that is the reason why I placed it in the test file rather than conftest. Suggestions welcome!
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def authorization2(route2, blame, cluster, label): |
There was a problem hiding this comment.
This should also be in auth conftest
There was a problem hiding this comment.
Same as above. For one test this auth policy is attached to HTTPRoute, for the other it is attached to GW. Again, not sure how to handle this nicely so that it can be shared in both tests.
|
Great test rewrite! To sum up my comments here. Please move |
The names of test functions are not ideal, a lot of duplicit info there, I agree. The reason I have it this way is that it helps to identify the test in the report portal. RP only shows test function name, that's why it is this long. However, you are right that 'identical_hostnames' is redundant in the name of the test files. I will rename. |
Signed-off-by: Tomas Repel <[email protected]>
trepel
force-pushed
the
identical-hostname-test-updates
branch
from
fbd9260 to
430cc7a
Compare
Overview
The 'identical hostname' limitation is no longer there. It should work the same as if the hostnames were not identical.
What was done
Removed the
*_desired.pytests since the desired behavior is now validated by the remaining tests. These tests were moved toauthandrlpdirectories and updated so that they validates the expected behavior and not the behavior with 'identical hostname' limitation.Verification Steps
make ./testsuite/tests/singlecluster/identical_hostnamesAll four tests should pass