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

filter Unit test #8111

Merged
merged 11 commits into from
Aug 5, 2023
Merged

filter Unit test #8111

merged 11 commits into from
Aug 5, 2023

Conversation

Philip-21
Copy link
Member

@Philip-21 Philip-21 commented Jul 9, 2023

Notes for Reviewers

This PR fixes for issue #8087
It contains unit test for the filter.go in mesheryctl

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@welcome
Copy link

welcome bot commented Jul 9, 2023

Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in 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.

@github-actions github-actions bot added the component/mesheryctl CLI for Meshery label Jul 9, 2023
@Philip-21 Philip-21 changed the title filter-test filter Unit test Jul 9, 2023
@github-actions
Copy link

github-actions bot commented Jul 9, 2023

@Philip-21 Philip-21 self-assigned this Jul 9, 2023
@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2ed3712) 5.22% compared to head (c8f3a97) 5.22%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #8111   +/-   ##
======================================
  Coverage    5.22%   5.22%           
======================================
  Files         124     124           
  Lines       17522   17522           
======================================
  Hits          915     915           
  Misses      16435   16435           
  Partials      172     172           
Flag Coverage Δ
gointegrationtests 5.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
…reating response files

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Member Author

Philip-21 commented Jul 10, 2023

@leecalcote @alphaX86 @nebula-aac @hexxdump @abdullah1308
I followed the mesheryctl's plan of mocking unit test from this discussion.
The tests coverage keeps failing . The error is shown here , i'm still figuring out the error, but i am just giving you all a heads up on it ,i would also appreciate some help .

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@abdullah1308
Copy link
Contributor

@Philip-21 Hey I have a PR making a few changes in mesheryctl filter here - #8063.

Once that is merged, make sure these tests still work.

@Chadha93
Copy link
Member

@Philip-21 Let's discuss this on the Meshery Dev call. Please add this as an agenda item in the meeting minutes if you would. :)

@Philip-21
Copy link
Member Author

@Philip-21 Hey I have a PR making a few changes in mesheryctl filter here - #8063.

Once that is merged, make sure these tests still work.

I've looked at the pr adding support for WASM filters is a great idea, it seems it will tackle the Error processing JSON response from server, because i also ran the other testcases locally, they failed and gave the same error .

…e directory

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@Philip-21
Copy link
Member Author

Philip-21 commented Jul 13, 2023

ci-checks

My Ci checks have passed on codecov with these latest commit, i added more testcases , I would like reviews on this before it gets merged

@Philip-21
Copy link
Member Author

@leecalcote ,@abdullah1308 , @Chadha93 👀👀👀

@leecalcote
Copy link
Member

leecalcote commented Jul 18, 2023

@abdullah1308 do you mind offering review here? 🙏

@leecalcote leecalcote requested review from abdullah1308 and removed request for alphaX86 July 18, 2023 15:22
}{
{
Name: "filter viewcmd with name",
Args: []string{"view", "view-filter-name"},
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add tests to use filter viewcmd with valid name and some non-existing filtername?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

},
{
Name: "filter viewcmd with ID",
Args: []string{"view", "c0c6035a-b1b9-412d-aab2-4ed1f1d51f84"},
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a test for invalid ID as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
hexxdump
hexxdump previously approved these changes Jul 21, 2023
Copy link
Contributor

@hexxdump hexxdump left a comment

Choose a reason for hiding this comment

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

LGTM

@Philip-21
Copy link
Member Author

@leecalcote everything is good with this Pr.

Signed-off-by: Philip-21 <philipuzomaobiora@gmail.com>
@leecalcote leecalcote merged commit f269378 into meshery:master Aug 5, 2023
16 of 18 checks passed
@Philip-21 Philip-21 deleted the filter-test branch September 18, 2023 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/mesheryctl CLI for Meshery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants