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

Added unit test for describe #521

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ayushrakesh
Copy link

@ayushrakesh ayushrakesh commented Jun 28, 2024

Added unit test for describe package in kubernetes via mock client

This PR fixes #287

Only added test for describe package, will soon add tests for expose and kompose

Signed commits

  • Yes, I signed my commits.

Signed-off-by: ayushrakesh <[email protected]>
Copy link

welcome bot commented Jun 28, 2024

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, please review the Layer5 Community Welcome Guide and sure to join the community Slack.
Be sure to double-check that you have signed your commits. Here are instructions for making signing an implicit activity while peforming a commit.

@Aisuko Aisuko added the area/tests Testing / quality assurance label Jun 30, 2024
@Aisuko Aisuko self-requested a review June 30, 2024 06:37
Copy link
Member

@Aisuko Aisuko left a comment

Choose a reason for hiding this comment

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

Thanks for your contributing, why there are so many *.json files?

cc @Yashsharma1911

@ayushrakesh
Copy link
Author

@Aisuko That's what I can't able to understand ,why this is so, I have tried by removing my cloned copy and cloning it again, but it didn't worked. Can you help?

@Philip-21
Copy link
Member

Philip-21 commented Jul 2, 2024

@ayushrakesh declare a t.tempdir() to remove to test files or generated files after execution to keep things tidy,
by the way does the test pass ?

@ayushrakesh
Copy link
Author

@ayushrakesh declare a t.tempdir() to remove to test files or generated files after execution to keep things tidy, by the way does the test pass ?

@Philip-21 Sorry, but I can't understood what you said. Please can you explain once more? Test passed successfully by running make test.

@Philip-21
Copy link
Member

if tests pass successfully, then its fine. Use the temp.Dir() or os package to remove the generated test files for this unit test you wrote

@ayushrakesh
Copy link
Author

@Philip-21 Pr is revised.

@ayushrakesh
Copy link
Author

@Philip-21 Please review my Pr.

}

// DescribeWithMock allows injection of a custom DescriberFor function
func DescribeWithMock(client KubeClient, options DescriberOptions, describerFor func(schema.GroupKind, *rest.Config) (describe.ResourceDescriber, bool)) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The Describe function is almost re-defined (refer to the describe.go) except for the type of client; hence, the test is of little to no value.
The test is testing the DescribeWithMock instead of testing the Describe defined in describe.go.

If the Describe function changes, how will this test help validate its functioning?

// Create a temporary file to simulate test-generated files
tempFile := filepath.Join(tempDir, "tempfile.txt")
if err := os.WriteFile(tempFile, []byte("temporary content"), 0644); err != nil {
t.Fatalf("Failed to write to temporary file: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the creation of a temp file drives the results of the tests?
Suppose describe gives the correct o/p but fails to write to the file, does it mean Describe function is breaking, I don't think so.

Copy link

stale bot commented Aug 25, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the issue/stale Issue has not had any activity for an extended period of time label Aug 25, 2024
@Ashparshp Ashparshp added issue/willfix This issue will be worked on issue/remind Issue progress check and removed issue/stale Issue has not had any activity for an extended period of time labels Aug 27, 2024
Copy link

Checking in... it has been awhile since we've heard from you on this issue. Are you still working on it? Please let us know and please don't hesitate to contact a MeshMate or any other community member for assistance.


        Be sure to join the community, if you haven't yet and please leave a ⭐ star on the project 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tests Testing / quality assurance issue/remind Issue progress check issue/willfix This issue will be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing Unit tests For k8 in Utils package
5 participants