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

Improve code coverage in satellite/console #6755

Open
4 of 9 tasks
mobyvb opened this issue Feb 6, 2024 · 13 comments
Open
4 of 9 tasks

Improve code coverage in satellite/console #6755

mobyvb opened this issue Feb 6, 2024 · 13 comments
Assignees

Comments

@mobyvb
Copy link
Member

mobyvb commented Feb 6, 2024

This ticket is designed to be completed in one sprint by the Console team. Individual items on the checklist do not need to be converted to issues, and can simply be checked when merged.

Instructions for working on tasks:

If you want to "claim" a checklist item, please edit the issue and put your username beside it, so that no one else tries to work on the same thing.

If the function is an unused feature, remove it. Otherwise, add testing to it. After merging your changes, check the item in the list.

If the function needing testing spans across multiple packages (e.g. consoleweb and console), it is sufficient to add a single test that covers both. It is also fine to add separate tests for each package (e.g. test important logic in console and test http/api-specific stuff in consoleweb)

Cover major edge cases, but don't worry about covering every single case for these tasks. This will be a long-term effort and we will likely make multiple passes as coverage improves.

Instructions for generating coverage report:

  1. run go test -coverprofile cover.out ./satellite/console/...
  2. go tool cover -html=cover.out to view an html version of the report

Tasks for this body of work:

  • SetupAccount() in console/service.go and consoleapi/auth.go @mobyvb
  • GetUsageReport() in console/service.go @VitaliiShpital
  • GetProjectUsage() in console/service.go @mobyvb
  • GetBucketTotals() in console/service.go
  • UpdateAccount() in console/service.go and consoleapi/auth.go @wilfred-asomanii
  • AttemptPayOverdueInvoices() in console/service.go @wilfred-asomanii
  • DeleteAccount() in console/service.go @VitaliiShpital
  • BillingHistory() in console/service.go and consoleapi/payments.go
  • InvoiceHistory() in console/service.go and consoleapi/payments.go

This is not an exhaustive list - do not add new items to it.

@mobyvb mobyvb added Needs Grooming Needs Estimation Issue still needs story pointing and removed Needs Grooming labels Feb 6, 2024
@NiaStorj NiaStorj removed the Needs Estimation Issue still needs story pointing label Feb 13, 2024
@storj-gerrit
Copy link

storj-gerrit bot commented Feb 15, 2024

Change satellite/console: test update account mentions this issue.

@wilfred-asomanii
Copy link
Contributor

Looks like there's a test, TestPayOverdueInvoices in invoices_test.go that tests AttemptPayOverdueInvoices
See

func TestPayOverdueInvoices(t *testing.T) {

@mobyvb
Copy link
Member Author

mobyvb commented Feb 15, 2024

@wilfred-asomanii thanks for finding that
maybe for payments/stripe-related wrapper functions, we can skip them if there is an adequate test in the stripe package. In this case I think it's okay because the only thing consoleService.AttemptPayOverdueInvoices does is call stripeService.AttemptPayOverdueInvoices. No additional logic is introduced, besides the audit log that happens in every console service function.

storjBuildBot pushed a commit that referenced this issue Feb 16, 2024
This change adds tests to the update account endpoint.

Issue: #6755

Change-Id: I77dd0100cb75aa1da07b77be0795913722ad3fe4
@egonelbre
Copy link
Member

egonelbre commented Feb 19, 2024

To get the latest coverage report it's also possible to see it in Jenkins https://build.dev.storj.tools/job/storj/job/main/cobertura. Although, looks like the tests haven't fully succeeded recently.

Also note that go test -coverprofile cover.out ./satellite/console/... may miss some of the coverage, because sometimes some other packages also cover code in satellite/console -- though, I'm not sure how common it is for satellite/console. For example, many things in satellite/satellitedb are tested from other packages.

@mobyvb
Copy link
Member Author

mobyvb commented Feb 19, 2024

@egonelbre @wilfred-asomanii already noticed that with one of the tests he looked into - AttemptPayOverdueInvoices is not tested in the console package, but it is tested in the payments package

@storj-gerrit
Copy link

storj-gerrit bot commented Feb 20, 2024

Change satellite/console: add GetUsageReport service test mentions this issue.

@storj-gerrit
Copy link

storj-gerrit bot commented Feb 20, 2024

Change satellite/console: remove /account/delete endpoint mentions this issue.

@VitaliiShpital
Copy link
Member

GetProjectUsage() in console/service.go
and
GetBucketTotals() in console/service.go

use methods which are very well tested in satellite/accounting/projectusage_test.go

storjBuildBot pushed a commit that referenced this issue Feb 20, 2024
added go unit test for GetUsageReport service method

Issue:
#6755

Change-Id: I33554345a99fffc7c5ebccb7d4d868da00fc35e1
storjBuildBot pushed a commit that referenced this issue Feb 21, 2024
Remove delete account endpoint as it's not used.

Issue:
#6755

Change-Id: I9faa2b59e8de92b8e51d0546b15909cc57aec6b6
@egonelbre
Copy link
Member

Ok, I managed to recently get one run https://build.dev.storj.tools/job/storj/job/main/4139/cobertura/

@mobyvb
Copy link
Member Author

mobyvb commented Feb 22, 2024

@egonelbre do you know if there is a way to view coverage of the line-by-line code from the report you shared? I'm only able to see high-level info, like what percentage of specific files are covered. I haven't figured out how to view the file itself to see which lines are covered

@egonelbre
Copy link
Member

@mobyvb, unfortunately that one no. I think I tried at some point using something, but the sizes were quite large for that.

Maybe we can just persist the coverage as an artifact, and then people can open them up locally somehow. Not sure whether we need to fix some paths, but should be doable.

@egonelbre
Copy link
Member

I created https://review.dev.storj.io/c/storj/storj/+/12527 for now. There also seem to be Jenkins plugins that can show more detailed information https://plugins.jenkins.io/coverage, but currently that install failed.

@egonelbre
Copy link
Member

The builds now contain the compressed coverage profile https://build.dev.storj.tools/blue/organizations/jenkins/storj/detail/main/4157/artifacts/

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

No branches or pull requests

7 participants