chore: added tests for degraphql_routes entity #155

Prashansa-K · 2024-12-20T09:56:59Z

Summary

Tests added for new entity: degraphql_routes

Testing

Unit tests
E2E tests
Manual testing on Universal
Manual testing on Kubernetes

codecov-commenter · 2024-12-20T09:59:26Z

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 28.11%. Comparing base (2772231) to head (3c09af0).

Files with missing lines	Patch %	Lines
pkg/file/builder.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   27.27%   28.11%   +0.83%     
==========================================
  Files         109      109              
  Lines       16967    16976       +9     
==========================================
+ Hits         4627     4772     +145     
+ Misses      11851    11697     -154     
- Partials      489      507      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pmalek

Minor nits, otherwise this looks good.

pmalek · 2025-01-08T09:23:17Z

pkg/file/builder_test.go

+				targetContent: tt.fields.targetContent,
+				currentState:  tt.fields.currentState,
+			}
+			b.build()


Any reason we don't check the error returned from build()?

pmalek · 2025-01-08T09:23:58Z

pkg/file/builder_test.go

@@ -3853,3 +3870,151 @@ func Test_stateBuilder_expressionRoutes_kong370_withKonnect(t *testing.T) {
 		})
 	}
 }
+
+func Test_stateBuilder_ingestPluginEntities(t *testing.T) {
+	rand.Seed(42)


This is deprecated as per: https://pkg.go.dev/math/rand#Seed

Deprecated: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator.

I have added changes based on the recommendations in rand package. We need that for deterministically generating uuids for the entities.

pmalek · 2025-01-08T09:25:10Z

pkg/state/degraphql_route_test.go

+	degraphqlRoute.Methods = kong.StringSlice("GET")
+
+	err := collection.Add(degraphqlRoute)
+	assert.Nil(err)


Nit: I'd consider using require for these errors, WDYT?

pmalek · 2025-01-08T09:27:04Z

pkg/state/degraphql_route_test.go

+	assert.IsType([]*DegraphqlRoute{}, degraphqlRoutes)
+}
+
+func populateDegraphqlRoutes(assert *assert.Assertions,


Nit: this is fine but I'd consider passing the t *testing.T instead of the assert object as using the latter can potentially (not necessarily) result in an unrelated assert being used.

IMHO the former is also more idiomatic. WDYT?

pmalek

Minor nits here and there for consideration

pmalek · 2025-01-23T13:31:31Z

pkg/file/builder_test.go

@@ -389,10 +390,29 @@ func existingFilterChainState() *state.KongState {
 	return s
 }

+func existingDegraphqlRouteState() *state.KongState {
+	s, _ := state.NewKongState()


Why not check the error? I'd suggest we consider adding a *testing.T param and require.NoError(t, err) here (as well as t.Helper() to make sure the correct call site of existingDegraphqlRouteState is printed in the logs). WDYT?

pkg/file/builder_test.go

pkg/state/degraphql_route_test.go

pmalek · 2025-01-23T13:37:31Z

pkg/state/degraphql_route_test.go

+	var degraphqlRoute DegraphqlRoute
+	degraphqlRoute.ID = kong.String("example")
+	degraphqlRoute.URI = kong.String("/foo")
+	degraphqlRoute.Query = kong.String("query { hello }")
+	degraphqlRoute.Service = &kong.Service{
+		ID: kong.String("some-service"),
+	}
+	degraphqlRoute.Methods = kong.StringSlice("GET")


Any reason not to instantiate it like so:

Suggested change

var degraphqlRoute DegraphqlRoute

degraphqlRoute.ID = kong.String("example")

degraphqlRoute.URI = kong.String("/foo")

degraphqlRoute.Query = kong.String("query { hello }")

degraphqlRoute.Service = &kong.Service{

ID: kong.String("some-service"),

}

degraphqlRoute.Methods = kong.StringSlice("GET")

var degraphqlRoute := DegraphqlRoute{

...

}

?

Prashansa-K mentioned this pull request Dec 20, 2024

feat: added support for degraphql_routes #154

Merged

5 tasks

pmalek reviewed Jan 8, 2025

View reviewed changes

Prashansa-K force-pushed the tests/degraphql-routes branch from e97c19e to 00bc3e3 Compare January 22, 2025 08:59

Prashansa-K added 4 commits January 22, 2025 18:06

feat: added support for degraphql_routes

ea26b37

chore: fixed lint issues

623b91f

chore: fixed code comments

08e1970

chore: addressed PR comments

a01aa3f

Prashansa-K force-pushed the feat/degraphql-routes branch from 2900111 to a01aa3f Compare January 22, 2025 12:36

Prashansa-K force-pushed the tests/degraphql-routes branch from 00bc3e3 to 8946072 Compare January 22, 2025 13:31

fix: renamed plugin_entities to custom_entities for clarity

d332712

Prashansa-K force-pushed the tests/degraphql-routes branch from 435ebd8 to aeb7d0f Compare January 23, 2025 09:25

fix: fixed comment strings

fb9c9fe

Prashansa-K force-pushed the tests/degraphql-routes branch from aeb7d0f to 3bcd119 Compare January 23, 2025 09:36

Prashansa-K added 5 commits January 23, 2025 15:11

fix: fixed schema

dedb146

chore: added tests for degraphql_routes entity

ade043d

test: fixed tests based on PR suggestions

14da3fc

test: added integration test for degraphql routes

da1406f

fix: fixed testcases based on renames

1ce0758

Prashansa-K force-pushed the tests/degraphql-routes branch from 3bcd119 to 1ce0758 Compare January 23, 2025 09:42

Base automatically changed from feat/degraphql-routes to main January 23, 2025 12:48

Prashansa-K added 2 commits January 23, 2025 18:44

Merge branch 'main' into tests/degraphql-routes

1cccb0e

style: lint fix

bcc50a3

Prashansa-K requested a review from pmalek January 23, 2025 13:29

pmalek reviewed Jan 23, 2025

View reviewed changes

fix: added PR suggestions

3c09af0

Prashansa-K requested a review from pmalek January 23, 2025 13:58

pmalek approved these changes Jan 23, 2025

View reviewed changes

Prashansa-K merged commit c296111 into main Jan 23, 2025
24 checks passed

Prashansa-K deleted the tests/degraphql-routes branch January 23, 2025 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: added tests for degraphql_routes entity #155

chore: added tests for degraphql_routes entity #155

Prashansa-K commented Dec 20, 2024 •

edited

Loading

codecov-commenter commented Dec 20, 2024 •

edited

Loading

pmalek left a comment

pmalek Jan 8, 2025

pmalek Jan 8, 2025

Prashansa-K Jan 22, 2025

pmalek Jan 8, 2025

pmalek Jan 8, 2025

pmalek left a comment

pmalek Jan 23, 2025

pmalek Jan 23, 2025

chore: added tests for degraphql_routes entity #155

chore: added tests for degraphql_routes entity #155

Conversation

Prashansa-K commented Dec 20, 2024 • edited Loading

Summary

Testing

codecov-commenter commented Dec 20, 2024 • edited Loading

Codecov Report

pmalek left a comment

Choose a reason for hiding this comment

pmalek Jan 8, 2025

Choose a reason for hiding this comment

pmalek Jan 8, 2025

Choose a reason for hiding this comment

Prashansa-K Jan 22, 2025

Choose a reason for hiding this comment

pmalek Jan 8, 2025

Choose a reason for hiding this comment

pmalek Jan 8, 2025

Choose a reason for hiding this comment

pmalek left a comment

Choose a reason for hiding this comment

pmalek Jan 23, 2025

Choose a reason for hiding this comment

pmalek Jan 23, 2025

Choose a reason for hiding this comment

Prashansa-K commented Dec 20, 2024 •

edited

Loading

codecov-commenter commented Dec 20, 2024 •

edited

Loading