Skip to content

cmds/core/tee add test coverage#2542

Merged
probot-auto-merge[bot] merged 6 commits intou-root:mainfrom
binjip978:tee-tests
Nov 17, 2022
Merged

cmds/core/tee add test coverage#2542
probot-auto-merge[bot] merged 6 commits intou-root:mainfrom
binjip978:tee-tests

Conversation

@binjip978
Copy link
Copy Markdown
Contributor

@binjip978 binjip978 commented Oct 29, 2022

Signed-off-by: Siarhiej Siemianczuk [email protected]

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 29, 2022 •
edited
Loading

Codecov Report

Base: 73.68% // Head: 73.68% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (83ac8a2) compared to base (18cc104).
Patch coverage: 72.00% of modified lines in pull request are covered.

❗ Current head 83ac8a2 differs from pull request most recent head 18c9d83. Consider uploading reports for the commit 18c9d83 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2542      +/-   ##
==========================================
- Coverage   73.68%   73.68%   -0.01%     
==========================================
  Files         406      407       +1     
  Lines       41299    41316      +17     
==========================================
+ Hits        30433    30442       +9     
- Misses      10866    10874       +8
Impacted Files Coverage Δ
cmds/core/tee/tee.go 76.47% <72.00%> (ø)
pkg/dt/fdt.go 46.54% <0.00%> (-4.31%) ⬇️
pkg/dt/fdt_linux.go 100.00% <0.00%> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@binjip978
Copy link
Copy Markdown
Contributor Author

binjip978 commented Oct 29, 2022

Looks like code coverage is missing for all cmds packages that don't have any tests, for example tar is not included into report as well.

rminnich
rminnich reviewed Oct 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

Thanks for the tests, can you consider my comments and see what you think? I'm interested in your ideas.

@rminnich rminnich added the Awaiting author Waiting for new changes or feedback for author. label Oct 29, 2022
rminnich
rminnich reviewed Oct 30, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

with that very minor nit, this looks like a real improvement over the original! thanks.

@binjip978 binjip978 requested a review from rminnich October 31, 2022 08:03
abrender
abrender reviewed Oct 31, 2022
View reviewed changes
abrender
abrender reviewed Nov 8, 2022
View reviewed changes
@binjip978
cmds/core/tee add test coverage
d184a29 
Signed-off-by: Siarhiej Siemianczuk <[email protected]>
@binjip978
cmds/core/tee: add command struct
9148bde 
struct command should remove dependency on flag pointer variables and
simplify testing

Signed-off-by: Siarhiej Siemianczuk <[email protected]>
@abrender
Copy link
Copy Markdown
Contributor

abrender commented Nov 9, 2022

Looks good to me, thanks! I'll leave it to @rminnich to resolve his open comments

@binjip978 binjip978 marked this pull request as draft November 13, 2022 13:26
@binjip978 binjip978 marked this pull request as ready for review November 13, 2022 13:26
rminnich
rminnich approved these changes Nov 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

One last quick question: did you run the command by hand and verify it still works? Believe it or not we've had a few issues with such an event. Thanks.

rminnich
rminnich approved these changes Nov 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rminnich rminnich left a comment

Choose a reason for hiding this comment

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

it works fine for me however.

@binjip978
Copy link
Copy Markdown
Contributor Author

binjip978 commented Nov 15, 2022

One last quick question: did you run the command by hand and verify it still works? Believe it or not we've had a few issues with such an event. Thanks.

Yeah, but it was some time ago. Anyway if any bug will be found assign to me :)

@rminnich rminnich added the automerge Applying this label auto-merges the PR when ready label Nov 15, 2022
@probot-auto-merge probot-auto-merge bot merged commit 8ed58e6 into u-root:main Nov 17, 2022
@binjip978 binjip978 deleted the tee-tests branch November 15, 2023 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abrender abrender abrender left review comments

@rminnich rminnich rminnich approved these changes

Assignees

No one assigned

Labels

automerge Applying this label auto-merges the PR when ready Awaiting author Waiting for new changes or feedback for author.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@binjip978 @abrender @rminnich