Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge TLS and OIDC e2e tests #7638

Open
creydr opened this issue Jan 29, 2024 · 4 comments
Open

Merge TLS and OIDC e2e tests #7638

creydr opened this issue Jan 29, 2024 · 4 comments
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@creydr
Copy link
Member

creydr commented Jan 29, 2024

Currently the TLS and OIDC e2e tests are mostly redundant and test the same (e.g. both have test to test the reply & DLS). Mostly the only difference is, that the OIDC tests configure an audience.

Therefor we should check, which OIDC and TLS tests can be merged and merge both "suites" together.

Hint:

  • Some tests still might be only relevant for TLS or OIDC (e.g. TestBrokerSupportsOIDC which tests only for OIDC compliance)
@creydr creydr added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 29, 2024
@parth721
Copy link

Hi @creydr, I am interested in this issue and want to gain more experience in this topic as a beginner.

@creydr
Copy link
Member Author

creydr commented Feb 19, 2024

Hello @parth721,
thanks for showing interest in this issue 👍
Anyhow, in case you're new to knative or the code base, I think a good-first-issue could be a good start to get into the code bases and the development process. You can check for good-first-issues e.g. here: https://github.com/search?q=org%3Aknative+org%3Aknative-extensions++label%3A%22good+first+issue%22+is%3Aissue+is%3Aopen+&type=issues.
In case you want to "prepare" for these issues, check for TLS or OIDC related issues.

Anyhow if you feel confident for this issue already, feel free to assign it to you.

@parth721
Copy link

parth721 commented Feb 19, 2024

Hey @creydr,
you are right, picking a goodfirst issues to understand the codebase is a great idea, but the catch is that all the issues are assigned, and some issues are merged anywhere else but kept open as goood first issue( 1yr old).
Are these issues still kept opened for beginners or is there any labels similar to good first issue(level 2) :).
what about difficulty level of flaky issues ? I don't know much about them, but i'm happy to learn.

@creydr creydr added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Apr 11, 2024
@prajjwalyd
Copy link
Contributor

Hey @creydr,
I was going through the tests yesterday and noticed that TestMTChannelBrokerRotateTLSCertificates and TestBrokerSupportsOIDC from broker_test.go, as well as (to some extent) TestInMemoryChannelTLS and TestChannelImplSupportsOIDC from channel_test.go, have a significant overlap in environment setup and focus on similar functionalities.
I think merging these tests into single functions with clear subtests would indeed streamline our test suite...
I'm looking to fix this issue and wanted to confirm if my identification is correct or if there are some other aspects left to consider.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
Status: No status
Status: No status
Development

No branches or pull requests

3 participants