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

fix: test for delete filter command #8396

Merged
merged 1 commit into from
Aug 16, 2023
Merged

fix: test for delete filter command #8396

merged 1 commit into from
Aug 16, 2023

Conversation

Yana-Gupta
Copy link
Member

@Yana-Gupta Yana-Gupta commented Aug 6, 2023

Notes for Reviewers

This PR fixes failing test in delete_test.go

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Yana-Gupta <yanagupta897@gmail.com>
@github-actions github-actions bot added the component/mesheryctl CLI for Meshery label Aug 6, 2023
@Yana-Gupta Yana-Gupta changed the title added space for test acceptance fix: test for delete filter command Aug 6, 2023
@github-actions
Copy link

github-actions bot commented Aug 6, 2023

@Yana-Gupta
Copy link
Member Author

Yana-Gupta commented Aug 6, 2023

@theBeginner86

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

Gosh, I can't quite tell what the change is here...

@Yana-Gupta
Copy link
Member Author

Yana-Gupta commented Aug 8, 2023

Gosh, I can't quite tell what the change is here...

Added space empty space \n to match both expected and actual responses, the actual response contains space at end but the expected doesn't so, by adding an empty single line at the end of those expected responses from delete.kuma.output.golden and delete.rollout.output.golden from where those strings(expected response) are coming the test cases will get passed.

Copy link
Member

@Philip-21 Philip-21 left a comment

Choose a reason for hiding this comment

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

@Yana-Gupta with or without these spaces the testcase still passes

@Yana-Gupta
Copy link
Member Author

some tests are failing here

@zakisk
Copy link
Contributor

zakisk commented Aug 10, 2023

@Yana-Gupta are copying the output from the terminal to the golden file?

@zakisk
Copy link
Contributor

zakisk commented Aug 10, 2023

@Yana-Gupta if so then copying output from terminal to the golden file doesn't copy the exact output generated by CLI. in order to get exact output in golden file use output operator > in terminal do this, (assuming you're in meshery/mesheryctl/ directory) ./mesheryctl filter delete c0c6035a-b1b9-412d-aab2-4ed1f1d51f84 Kuma-Test > ./internal/cli/root/filter/fixtures/delete.kuma.output.golden. it will store the exact output in the output golden file.

@Yana-Gupta
Copy link
Member Author

@Yana-Gupta if so then copying output from terminal to the golden file doesn't copy the exact output generated by CLI. in order to get exact output in golden file use output operator > in terminal do this, (assuming you're in meshery/mesheryctl/ directory) ./mesheryctl filter delete c0c6035a-b1b9-412d-aab2-4ed1f1d51f84 Kuma-Test > ./internal/cli/root/filter/fixtures/delete.kuma.output.golden. it will store the exact output in the output golden file.

@zakisk Thanks for informing

Copy link
Member

@leecalcote leecalcote left a comment

Choose a reason for hiding this comment

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

I still don't see a difference... what was changed here?

@leecalcote
Copy link
Member

@Yana-Gupta what did you change in this PR from what is in master?

@Yana-Gupta
Copy link
Member Author

Yana-Gupta commented Aug 16, 2023

@Yana-Gupta what did you change in this PR from what is in master?

only an empty line difference was changed, before it was not there in master I just added it.

@vishalvivekm
Copy link
Member

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

@Yana-Gupta
Copy link
Member Author

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

Thanks @vishalvivekm it was already discussed earlier on Meshery build and release call.

@leecalcote
Copy link
Member

@Yana-Gupta what did you change in this PR from what is in master?

only an empty line difference was changed, before it was not there in master I just added it.

Sounds real good. Or do you mean that you removed an extraneous line return?

@leecalcote leecalcote merged commit b42876f into meshery:master Aug 16, 2023
11 checks passed
@Philip-21
Copy link
Member

Philip-21 commented Aug 17, 2023

delete-test

@Yana-Gupta Thanks i see what you meant by this indent you made. The test passed with the additional changes i made

@Yana-Gupta
Copy link
Member Author

@Yana-Gupta what did you change in this PR from what is in master?

only an empty line difference was changed, before it was not there in master I just added it.

Sounds real good. Or do you mean that you removed an extraneous line return?

I adjusted the lines to pass the test case.. but I have added space to pass the testcase in the testdata files.. I don't know why the changes are not same as of my local system

@Philip-21
Copy link
Member

@Yana-Gupta what did you change in this PR from what is in master?

only an empty line difference was changed, before it was not there in master I just added it.

Sounds real good. Or do you mean that you removed an extraneous line return?

I adjusted the lines to pass the test case.. but I have added space to pass the testcase in the testdata files.. I don't know why the changes are not same as of my local system

I was able to sort that out in this pr, but other changes are still ongoing in it

@Philip-21
Copy link
Member

Philip-21 commented Aug 17, 2023

@Yana-Gupta when working with text files or string data that might be used across different systems, newline representations are used in a specific context, newline representations can lead to compatibility issues, especially when comparing or processing texts across platforms.
Windows uses /r/n while Unix is /n so there can be compatibility issues.
It's why the changes you made were not effective at first

@Yana-Gupta Yana-Gupta deleted the filter-test branch August 20, 2023 08:53
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