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: improve backend testsuite #1120

Merged
merged 24 commits into from
Oct 27, 2023
Merged

fix: improve backend testsuite #1120

merged 24 commits into from
Oct 27, 2023

Conversation

Traxmaxx
Copy link
Contributor

@Traxmaxx Traxmaxx commented Oct 20, 2023

Problem

The testsuite in CI/CD and localy currently reports

Run make test
go test -v 
?   	github.com/tailwarden/komiser	[no test files]

Solution

Tests are executed like go test -v $(go list ./... | grep -v /dashboard/) and this is suppossed to work fine, however for unknown reasons it does not when executed through the Makefile. Updating the command to go test ./... | grep -v /dashboard/ and pipe the ignore filter instead works more reliably

Changes Made

  • Update Makefile to execute all found tests except inside /dashboard folder
  • Update GCP pricing map (SEE NOTES )
  • Update CONTRIBUTING.md
  • Update tests machine_test.go and refactor how to test because previously we were mapping against a fixed price. I presume (actually it's hope) that the schema changes less often than the prices. I made the tests more dry and only check if the price returned is above a certain number. The number is currently set to 0.
  • Update Feedback Widget to use new Toast component

How to Test

  • Localy: checkout this branch and run make test or go test ./... | grep -v /dashboard/
  • CI/CD: check the run results of the latest commit in this PR. Is should look similar to the attached screenshot!

Screenshots

Screenshot 2023-10-20 at 13 47 34
Screenshot 2023-10-20 at 13 25 03

Notes

There are test fixes in here as well.
For example /utils/gcpcomputepricing was broken. It seems like Google updated their pricing structure?
Comparing our current VmsOnDemandMemoryPerGb type with the content returned from gcp-compute.json it seems they now nest Vmimagee2RAME2 instance type under E2 (sdee screenshot attached).
I updated the types being used in the utils test and the test now passes without error.

Before the tests where returning

--- FAIL: TestGet (0.32s)
panic: runtime error: index out of range [0] with length 0 [recovered]
	panic: runtime error: index out of range [0] with length 0
--- FAIL: TestGetE2standard32OnDemand (0.08s)
    machine_test.go:62: core price not found for "us-west1" region

Also check comment thread please.

Checklist

  • Code follows the contributing guidelines
  • Changes have been thoroughly tested
  • Documentation has been updated, if necessary
  • Any dependencies have been added to the project, if necessary

Reviewers

@[username of the reviewer]

@Traxmaxx Traxmaxx changed the title fix: change test command to look for tests in all folders except dash… fix: improve backend testsuite Oct 20, 2023
@Traxmaxx Traxmaxx added bug Something isn't working help wanted Extra attention is needed go Pull requests that update Go code cli gcp labels Oct 20, 2023
@shavidze
Copy link
Contributor

Can I help you?

@Traxmaxx
Copy link
Contributor Author

@shavidze yes please. Do you think my observations are right? Or is there something else wrong with the GCP pricing tests?
I was unable to find any sort of changelog for https://www.gstatic.com/cloud-site-ux/pricing/data/gcp-compute.json

@Traxmaxx
Copy link
Contributor Author

Just for references: this PR introduced the above mentioned mapping and it seemed to have worked fine back then #652

@shavidze
Copy link
Contributor

@Traxmaxx I will take a look.

@Traxmaxx
Copy link
Contributor Author

@shavidze me and @mlabouardy came to the conclution that the pricemapping indeed changed. There are more types to update and we should rethink maybe if we really want to test all prices so detailed or if we should just check for a value greater than 0

@Traxmaxx
Copy link
Contributor Author

So that should be all for gcpcomputepricing 🤞

@Traxmaxx Traxmaxx marked this pull request as ready for review October 20, 2023 20:38
@Traxmaxx Traxmaxx self-assigned this Oct 20, 2023
@AvineshTripathi
Copy link
Collaborator

Hey @Traxmaxx there are some frontend changes too included in the PR please confirm if you want to include in this PR and let me know when I can test this... Excited!

@Traxmaxx
Copy link
Contributor Author

@AvineshTripathi its ready. I was just fixing test and lint errors in the frontend so the PR is green!

@Traxmaxx Traxmaxx removed the help wanted Extra attention is needed label Oct 23, 2023
Copy link
Collaborator

@AvineshTripathi AvineshTripathi left a comment

Choose a reason for hiding this comment

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

@Traxmaxx great work man! I didnt see anything more in that to review. Although I wanted to ask if we should move pricing logic of gcp inside something like /utils/pricing/gcp and structurize file structure in that more for maintaining

utils/gcpcomputepricing/machine.go Outdated Show resolved Hide resolved
@Traxmaxx
Copy link
Contributor Author

So what is the status of this PR? Anything else I should change?
@mlabouardy @AvineshTripathi @ShubhamPalriwala @jakepage91 ?

Copy link
Collaborator

@mlabouardy mlabouardy left a comment

Choose a reason for hiding this comment

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

All good :)

@mlabouardy mlabouardy added this to the v3.1.3 milestone Oct 27, 2023
@mlabouardy mlabouardy merged commit b4bae76 into develop Oct 27, 2023
3 checks passed
@mlabouardy mlabouardy deleted the feature/tech-1706 branch October 27, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli gcp go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants