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

Return empty array when using --json flag for list commands #736

Merged
merged 4 commits into from
Apr 13, 2023

Conversation

sergiught
Copy link
Contributor

🔧 Changes

When returning the output of a list command in combination with the --json flag, if there are no elements to display we should still return an empty array.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (59a0cad) 70.60% compared to head (2875a4a) 70.61%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #736   +/-   ##
=======================================
  Coverage   70.60%   70.61%           
=======================================
  Files          87       87           
  Lines       11197    11198    +1     
=======================================
+ Hits         7906     7907    +1     
  Misses       2778     2778           
  Partials      513      513           
Impacted Files Coverage Δ
internal/display/display.go 86.70% <100.00%> (+0.07%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sergiught sergiught marked this pull request as ready for review April 13, 2023 14:23
@sergiught sergiught requested a review from a team as a code owner April 13, 2023 14:23
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Should we add this assert to all other existing list integration tests?

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

Except for apps and apis, those will never be empty.

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

(by assert I mean the "contain" on "stdout").

Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Change makes sense and solves a very real pain point. Satisfied with the coverage provided by unit test + example integration test.

@sergiught sergiught merged commit f764ae5 into main Apr 13, 2023
@sergiught sergiught deleted the patch/empty-json-flag branch April 13, 2023 17:23
@sergiught
Copy link
Contributor Author

@Widcket we added unit tests for the renderer func + used it in 1 integration test. We felt it was enough for now :)

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

It's not about the coverage, it's about the other integration tests that did not have an assert on the output.

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

Of course, however you prefer.

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

This PR finally makes it possible to assert on the output of empty lists.

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

And there being a single function doing the output is an implementation detail, which is not relevant from the perspective of black-box integration tests.

@sergiught
Copy link
Contributor Author

@Widcket completely agreed, we can follow up in another PR to have those covered as well. We're mostly just trying to use our time wisely at the moment for the upcoming release. :)

@Widcket
Copy link
Contributor

Widcket commented Apr 13, 2023

Of course, prioritize as you must.

@willvedd willvedd mentioned this pull request Apr 19, 2023
2 tasks
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