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

test(RHTAPWATCH-1067): Unit tests for getUserNamespaces #44

Merged
merged 1 commit into from
Sep 1, 2024

Conversation

Kousalya1998
Copy link

Add unit test for func getUserNamespaces

@Kousalya1998 Kousalya1998 marked this pull request as draft July 23, 2024 20:36
@Kousalya1998 Kousalya1998 marked this pull request as ready for review July 23, 2024 20:54
@Kousalya1998 Kousalya1998 marked this pull request as draft July 23, 2024 22:28
@Kousalya1998 Kousalya1998 marked this pull request as ready for review July 24, 2024 14:03
Copy link
Collaborator

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

Overall good progress, but I am not sure why a change in the function itself is required

cmd/main.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
@Kousalya1998
Copy link
Author

Overall good progress, but I am not sure why a change in the function itself is required

I saw your comments on the PR thanks. The reason why I had to modify the main function was: take for eg to get specific user namespace test case- the test is expecting only one namespace (“test-tenant”) but is returning all three namespaces (test-tenant, test-tenant2, test-tenant3). I felt that the label selector is not working as expected and thats why I added filteredNamespaces func in the main.go.

@Kousalya1998
Copy link
Author

Kousalya1998 commented Jul 25, 2024

@Omeramsc I tried the method that you suggested

I think you get all the namespaces because all of them have those labels. I t makes sense since we created them to be used in the functional tests, if they would not have those labels they won't pass.It means we don't need to change the function, as it works as it should. what we need to change is the namespaces we create. or rather - create a new namespace (either for this test specifically or for the whole beforesuite, see what works better), that doesn't have this label. then you can see it is not returned.

@Kousalya1998
Copy link
Author

Kousalya1998 commented Jul 25, 2024

@Omeramsc I tried the method that you suggested

I think you get all the namespaces because all of them have those labels. I t makes sense since we created them to be used in the functional tests, if they would not have those labels they won't pass.It means we don't need to change the function, as it works as it should. what we need to change is the namespaces we create. or rather - create a new namespace (either for this test specifically or for the whole beforesuite, see what works better), that doesn't have this label. then you can see it is not returned.

PTAL again. test cases pass.

@hmariset
Copy link
Contributor

lgtm

@Kousalya1998 Kousalya1998 requested a review from Omeramsc July 26, 2024 16:21
Copy link
Collaborator

@Omeramsc Omeramsc 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 much better now; I added a few more comments

cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
@Kousalya1998 Kousalya1998 force-pushed the RHTAPWATCH-1067 branch 2 times, most recently from f77bade to a66abe7 Compare July 29, 2024 21:05
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
@Kousalya1998 Kousalya1998 force-pushed the RHTAPWATCH-1067 branch 2 times, most recently from a294a97 to becfb27 Compare July 31, 2024 22:20
@Kousalya1998 Kousalya1998 force-pushed the RHTAPWATCH-1067 branch 2 times, most recently from 04fe886 to aa98b2b Compare August 13, 2024 18:43
@Kousalya1998
Copy link
Author

@Omeramsc PTAL. Three test cases fail out of the 4 because the function getUserNamespaces appears to be returning all user namespaces, regardless of the specific request. The same error I said in the beginning.

Copy link
Collaborator

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

@Omeramsc PTAL. Three test cases fail out of the 4 because the function getUserNamespaces appears to be returning all user namespaces, regardless of the specific request. The same error I said in the beginning.

  1. The BeforeSuite is still applied to all tests. this is not desireable, I'll push a change seperating the unit tests from the functional tests.
  2. Probably a result of the former point, but in all tests you never created a namespace. the namespaces from the BeforeSuite are configured to pass the functional tests, so the fact they always return is a given. you shouldn't use them, you should create your own in the beforeEach
  3. the echo server however is not needed in the beforeEach
  4. resetting variables is in the afterEach is not needed, you need to reset (=remove) the namespaces you create.

If you have any more questions about why we use the beforeEach and afterEach for those tests and resource management, please don't hesitate to ask me before applying more changes to the code.

cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
@Kousalya1998
Copy link
Author

This PR is blocked by RHTAPWATCH-1237 Reorganize workspace-manager test suites.

@Kousalya1998 Kousalya1998 force-pushed the RHTAPWATCH-1067 branch 4 times, most recently from fa2b460 to 495c77b Compare August 20, 2024 20:19
cmd/main.go Show resolved Hide resolved
Copy link
Collaborator

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

Good catch, I'm glad to see it moving forward. added some small requests

cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main_test.go Outdated Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
cmd/main.go Show resolved Hide resolved
Add unit test for func
getUserNamespaces
Copy link
Collaborator

@Omeramsc Omeramsc left a comment

Choose a reason for hiding this comment

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

LGTM

@Omeramsc Omeramsc merged commit 785d47a into konflux-ci:main Sep 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants