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.

Sign up for GitHub

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

Jump to bottom

cmd package test improvements #258

Merged
merged 33 commits into from
Dec 7, 2023
Merged

cmd package test improvements #258

merged 33 commits into from
Dec 7, 2023

Conversation

shireenf-ibm
Copy link
Contributor

@shireenf-ibm shireenf-ibm commented Oct 31, 2023 •
edited
Loading

issue #110

  • tests improvements for cmd pkg :
  • in this pkg could not run tests in parallel - seems that cmd.Execute func can not be run in parallel (when try to run in parallel, tests fail , expected err/ output of a test returned as the actual of another)

@shireenf-ibm shireenf-ibm requested a review from adisos October 31, 2023 08:42
@shireenf-ibm shireenf-ibm mentioned this pull request Oct 31, 2023
Merged
@shireenf-ibm shireenf-ibm removed the request for review from adisos November 9, 2023 08:31
@shireenf-ibm shireenf-ibm requested a review from adisos November 9, 2023 09:54
@shireenf-ibm
Copy link
Contributor Author

hi @adisos ,
I think this PR is ready for review (although I am still learning the new refactoring changes)

-- on commit , I had to delete a test with json output , because its expected output file caused two other tests in connlist_test.go to fail (it tries to read the json output file as an input file for those tests and fails with an error message containing json: cannot unmarshal array into Go value of type unstructured.detector"

adisos
adisos reviewed Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

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

A few general comments:

  • Change Makefile to support the update flag for this pkg as well
  • Move json expected output file to some other place so that it does not interfere with the test you had to comment-out?

cmd/netpolicy/cmd/command_test.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm marked this pull request as draft November 14, 2023 12:38
@shireenf-ibm shireenf-ibm linked an issue Nov 16, 2023 that may be closed by this pull request
tests improvements #110
Closed
9 tasks
@shireenf-ibm shireenf-ibm mentioned this pull request Nov 16, 2023
Merged
@shireenf-ibm shireenf-ibm requested a review from adisos November 23, 2023 18:00
adisos
adisos reviewed Dec 2, 2023
View reviewed changes
pkg/cli/command_test.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/cli/command_test.go Outdated Show resolved Hide resolved
pkg/cli/command_test.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/cli/command_test.go Outdated Show resolved Hide resolved
pkg/cli/command_test.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested a review from adisos December 3, 2023 14:36
adisos
adisos requested changes Dec 6, 2023
View reviewed changes
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

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

few more comments

pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
pkg/internal/testutils/testutils.go Outdated Show resolved Hide resolved
@shireenf-ibm shireenf-ibm requested a review from adisos December 6, 2023 10:59
@shireenf-ibm shireenf-ibm requested a review from adisos December 6, 2023 12:20
@shireenf-ibm shireenf-ibm requested a review from adisos December 7, 2023 08:12
adisos
adisos requested changes Dec 7, 2023
View reviewed changes
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

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

Looks good!
One more comment: can you move the output_files dir to be not under tests dir (it is currently confusing as it is mixed with a long list of actual test dirs). Let it be a sibling of the tests dir, and rename to test_outputs.

@shireenf-ibm
Copy link
Contributor Author

Looks good! One more comment: can you move the output_files dir to be not under tests dir (it is currently confusing as it is mixed with a long list of actual test dirs). Let it be a sibling of the tests dir, and rename to test_outputs.

done

@shireenf-ibm shireenf-ibm merged commit e86bf5a into main Dec 7, 2023
4 checks passed
@shireenf-ibm shireenf-ibm deleted the issue_110_tests_improvements_cmd_pkg branch December 7, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisos adisos adisos approved these changes

Assignees
No one assigned
Labels
size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tests improvements
2 participants
@shireenf-ibm @adisos