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

chore: Refactor pkg/testutils package #2173

Conversation

Tomasz-Smelcerz-SAP
Copy link
Member

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP commented Jan 13, 2025

Description

Splitting the pkg/testutils package: first steps
I focused to move the test functions "closer" to the tests that make use of them.
I tried to not modify any code - it's just moving of files and necessary package imports changes.
Now instead of one big package that is imported by many different tests, we have three packages: the top one pkg/testutils and two "local" ones: tests/e2e/commontestutils and tests/integration/commontestutils.

Note for reviewer: There's much more we can do there. However I still think the #1085 issue is not specific enough - considering how low it was estimated.
I'd vote for closing it, and proceed with smaller, more focused tasks: #2174

Changes proposed in this pull request:

  • moved some files into new package: tests/e2e/commontestutils
  • moved some files into new package: tests/integration/commontestutils
  • adjusted linters configuration

Related issue(s)
See also: #1085

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP requested a review from a team as a code owner January 13, 2025 20:15
@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 13, 2025
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP changed the title refactor pkg/testutils package chore: Refactor pkg/testutils package Jan 14, 2025
Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks good to me, but I am uncertain about the left follow-ups. #2174 is about specific helpers for that test, fair enough.

But what do we do with the rest remaining in pkg/testutils? If closing the origin ticket here, I think we still need to lay out a plan for how to continue with that. At least, we should put it to another location, e.g. tests/commontestutils

@Tomasz-Smelcerz-SAP
Copy link
Member Author

Tomasz-Smelcerz-SAP commented Jan 14, 2025

fair point. Personally I think that solving #2174 will tell us more about desired structures of packages for all the tests, not just this one. I chose watcher_test.go just to narrow the scope and avoid insanely huge PR.
I focused mostly on e2e test.
My investigation lead me to the following conclusion:

  • We probably need to put every e2e test in it's own package (Poc: Find a way to improve tests/e2e/watcher_test.go #2174 will tell)
  • We probably need also to distribute the "helper" code into sub-packages - not necessarily the same as the packages containing the test scenarios (may be the same, may be different, may be some mix)

These two observations come mostly from the fact that current package structure results in the huge, unstructured "bags" of test code and this violates some well established software engineering practices: Like "Law of Demeter", "information hiding", "high cohesion" just to name the few.
So it's natural to think about splitting this into sub-packages. Because there is more than one way to do it, I decided we need a playground for trying out different approaches: #2174

Bottom line: I think the splitting pkg/testutils is still valid, I just don't know how to do it with respect to all our test suites: e2e, integration, unit. So instead of splitting the package and then trying to adjust the tests to use it, maybe we should use the bottom-up approach: start from the tests and slowly move the code (test by test). In the end we should end up with the pkg/testutils refactored.

Copy link
Contributor

@c-pius c-pius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, sounds like a good proposal to me. It is just be important to me that we can include explicitly in the PoC ticket that this should also include a proposal on the way forward with the pkg/testutils package.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 14, 2025
@kyma-bot kyma-bot removed the lgtm Looks good to me! label Jan 14, 2025
@kyma-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP enabled auto-merge (squash) January 14, 2025 16:29
@Tomasz-Smelcerz-SAP
Copy link
Member Author

Okay, sounds like a good proposal to me. It is just be important to me that we can include explicitly in the PoC ticket that this should also include a proposal on the way forward with the pkg/testutils package.

I've updated the #2174 . Now it's quite a big PoC :)

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP merged commit 47e294c into kyma-project:main Jan 14, 2025
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants