Remove double imports and some dead code #7445
Conversation
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7445 +/- ##
==========================================
+ Coverage 63.79% 63.83% +0.04%
==========================================
Files 416 416
Lines 23440 23434 -6
==========================================
+ Hits 14954 14960 +6
+ Misses 7225 7216 -9
+ Partials 1261 1258 -3 ☔ View full report in Codecov by Sentry. |
ytimocin
force-pushed
the
ytimocin/refactor/v1
branch
from
a3b3202
to
ffc12fa
Compare
ytimocin
had a problem deploying
to
functional-tests
GitHub Actions
Failure
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
ytimocin
force-pushed
the
ytimocin/refactor/v1
branch
from
ffc12fa
to
66cac7f
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
| @@ -179,18 +141,6 @@ func (cli *CLI) ApplicationShow(ctx context.Context, applicationName string) (st | |||
| return cli.RunCommand(ctx, args) | |||
| } | |||
|
|
|||
| // ApplicationList runs a command to list applications and returns the output as a string. It returns an | |||
| // error if the command fails. | |||
| func (cli *CLI) ApplicationList(ctx context.Context) (string, error) { | |||
There was a problem hiding this comment.
Are these covered elsewhere in the tests?
There was a problem hiding this comment.
These functions are not being referenced anywhere in the code. I am not sure if it is wise to keep these just in case if we need them one day. I believe that if we need, we can add them in future. As of now, it is showing up as deadcode when I run staticcheck. That is why I believe it is good to remove them.
There was a problem hiding this comment.
I was curious why don't we just use these functions to add test coverage unless they are being tested through other code paths, but doesn't look like we are testing these commands. I don't think it is in scope of this PR to add tests, but I have created this issue #7453, we can reference this PR if these functions need to be brought back.
Thanks for doing this cleanup!
There was a problem hiding this comment.
I was curious why don't we just use these functions to add test coverage unless they are being tested through other code paths, but doesn't look like we are testing these commands. I don't think it is in scope of this PR to add tests, but I have created this issue #7453, we can reference this PR if these functions need to be brought back.
Thanks for doing this cleanup!
Our coverage results don't include functional tests. To do that we'd have to collect coverage from inside the containers where Radius is running.
There was a problem hiding this comment.
I'm meant tests, not coverage results, I know coverage results are for unit tests :). What I'm trying to understand is that why don't we have functional tests for these commands. Looks like a bunch of helper functions were added to be able to write functional tests but were never used.
I should have written "use these functions to add tests", sorry for the confusion.
There was a problem hiding this comment.
Ah sorry. No idea why then :)
Signed-off-by: ytimocin <ytimocin@microsoft.com>
ytimocin
force-pushed
the
ytimocin/refactor/v1
branch
from
66cac7f
to
13aaefb
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
Type of change