Kuadrant / testsuite

3 stars 12 forks source link

Add Kuadrantctl tests #463

Closed pehala closed 1 month ago

pehala commented 2 months ago

Reviewable but not mergable due to a few outstanding bugs and enhancements: Gonna be merged even with temporary fixes due to outstandiong issues with kuadrantctl itself https://github.com/Kuadrant/kuadrantctl/issues/94, https://github.com/Kuadrant/kuadrantctl/issues/91, https://github.com/Kuadrant/kuadrantctl/commit/d198cc865a2bfa0fed621d0238280e17502deaaf

Fixes #434 #365

trepel commented 1 month ago

Sorry for long comment, I was lazy/unable to identify pieces of code that these bulletpoints relate to

1) running make kuadrantctl causes three gateway CRs being created instantly at once. Not sure if this can cause any potential issues or not, I just find it strange.

2) RLPs and AuthPolicies name is route-* (it is actually the same as the target HTTPRoute). Again, not sure if this can cause any issues but less confusing names would be better

3) Intermittent error on RLP test if executed via make kuadrantctl:

testsuite/tests/kuadrantctl/cli/test_simple_limit.py:49: in test_generate_limit
    responses.assert_all(status_code=200)
testsuite/httpx/__init__.py:93: in assert_all
    assert request.status_code == status_code, (
E   AssertionError: Status code assertion failed for request 3 out of 3 requests: Result[status_code=429] != 200
---------------------------------------------------------------------------------------- Captured log call ----------------------------------------------------------------------------------------
10:08:13 +0200 INFO:httpx:HTTP Request: GET http://hostname-trepel--qmv8-kuadrant.apps.kua-stage-single.osp.api-qe.eng.rdu2.redhat.com/anything "HTTP/1.1 200 OK"
10:08:13 +0200 INFO:httpx:HTTP Request: GET http://hostname-trepel--qmv8-kuadrant.apps.kua-stage-single.osp.api-qe.eng.rdu2.redhat.com/anything "HTTP/1.1 200 OK"
10:08:13 +0200 INFO:httpx:HTTP Request: GET http://hostname-trepel--qmv8-kuadrant.apps.kua-stage-single.osp.api-qe.eng.rdu2.redhat.com/anything "HTTP/1.1 429 Too Many Requests"

I suspect it is due to the fact that the RLPs are identical and HTTPRoute is re-used across the tests so there might be a clash time to time. Potential solution might be to make the duration shorter (10s or even less) or maybe re-create the HTTPRoute as well or maybe do not have the RLPs identical (might be enough to have some random string as part of the name or smt like that)

4) It does not work with kuadrantctl binary built from main branch. It fails generating HTTPRoute with an error: resource name may not be empty. Probably bug in kuadrantctl itself rather that this PR so just fyi

pehala commented 1 month ago
  1. running make kuadrantctl causes three gateway CRs being created instantly at once. Not sure if this can cause any potential issues or not, I just find it strange.

This is plausible because tests are running in parallel

2. RLPs and AuthPolicies name is route-* (it is actually the same as the target HTTPRoute). Again, not sure if this can cause any issues but less confusing names would be better

That is out of my control, I apply exactly what I get from the kuadrantctl and as far as I know there is not option to specify name

3. Intermittent error on RLP test if executed via make kuadrantctl:

I will investigate that one, it could be because that route

4. It does not work with kuadrantctl binary built from main branch. It fails generating HTTPRoute with an error: resource name may not be empty. Probably bug in kuadrantctl itself rather that this PR so just fyi

Will try it out, so far I only tested released version

jsmolar commented 1 month ago

tests/conftest.py should not contain kuadrant fixture

pehala commented 1 month ago

tests/conftest.py should not contain kuadrant fixture

I do not touch that file at all, not sure if the change needs to be done here

pehala commented 1 month ago

I would hesitate merging this if not working on both (once 0.2.4 is GA'd we can merge imo, not sure about the timeline though).

Sadly, that is not easily achievable. There is a breaking change (https://github.com/Kuadrant/kuadrantctl/pull/78) that changes a lot (where exactly the extension is specified) and I do not think it warrants the time investment and complexity needed to make it interchangable. However, we now have https://github.com/Kuadrant/kuadrantctl/releases/tag/v0.3.0, which is now GA

trepel commented 1 month ago

However, we now have https://github.com/Kuadrant/kuadrantctl/releases/tag/v0.3.0, which is now GA

Cool, I am happy to approve once you address the two comments then!

jsmolar commented 1 month ago

@azgabur please review. I want you to take a look at this PR.

pehala commented 1 month ago

Removed committed