feat: rewrite e2e test(test-e2e-route-with-management-fields) using ginkgo #1704

bisakhmondal · 2021-04-02T09:46:27Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

Related issues

#1500

Checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Jaycean · 2021-04-04T12:21:41Z

api/test/e2enew/route/route_with_management_fields_test.go

+					"upstream": {
+						"type": "roundrobin",
+						"nodes": [{
+							"host": "172.16.238.20",


nodes host please use the base.UpstreamIp

"nodes": { "` + base.UpstreamIp + `:1980": 1 }

Thanks. Done ✔️

api/test/e2enew/route/route_with_management_fields_test.go

Jaycean · 2021-04-04T12:23:54Z

api/test/e2enew/route/route_with_management_fields_test.go

+			Headers:      map[string]string{"Authorization": base.GetToken()},
+			ExpectStatus: http.StatusOK,
+		})
+	})


After the route is deleted, we need to check whether the route is deleted.

Done ✔️

bisakhmondal · 2021-04-04T13:00:01Z

ci fails to pull docker images
cc @Jaycean, @juzhiyuan, @nic-chen

juzhiyuan · 2021-04-04T15:14:53Z

ci fails to pull docker images
cc @Jaycean, @juzhiyuan, @nic-chen

ok, just retrigger

nic-chen · 2021-04-05T00:23:54Z

api/test/e2enew/route/route_with_management_fields_test.go

+			Method:       http.MethodGet,
+			Path:         "/hello",
+			Headers:      map[string]string{"Authorization": base.GetToken()},
+			ExpectStatus: http.StatusNotFound,


better to check respond body for each case

codecov-io · 2021-04-05T19:26:09Z

Codecov Report

Merging #1704 (85b01da) into master (9cf91b7) will decrease coverage by 20.33%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1704       +/-   ##
===========================================
- Coverage   72.63%   52.29%   -20.34%     
===========================================
  Files         133       38       -95     
  Lines        5740     2660     -3080     
  Branches      666        0      -666     
===========================================
- Hits         4169     1391     -2778     
+ Misses       1327     1081      -246     
+ Partials      244      188       -56

Flag	Coverage Δ
backend-e2e-test	`?`
backend-e2e-test-ginkgo	`?`
backend-unit-test	`52.29% <ø> (+0.03%)`	⬆️
frontend-e2e-test	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/internal/utils/version.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/filter/request_id.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/core/entity/entity.go	`0.00% <0.00%> (-100.00%)`	⬇️
api/internal/core/store/storehub.go	`0.00% <0.00%> (-71.03%)`	⬇️
api/internal/filter/cors.go	`0.00% <0.00%> (-66.67%)`	⬇️
api/internal/filter/schema.go	`0.00% <0.00%> (-55.47%)`	⬇️
api/internal/utils/consts/api_error.go	`0.00% <0.00%> (-50.00%)`	⬇️
api/internal/handler/data_loader/route_import.go	`27.41% <0.00%> (-37.50%)`	⬇️
api/internal/handler/handler.go	`42.59% <0.00%> (-35.19%)`	⬇️
api/internal/handler/schema/schema.go	`66.66% <0.00%> (-33.34%)`	⬇️
... and 119 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cf91b7...85b01da. Read the comment docs.

nic-chen · 2021-04-06T04:49:23Z

api/test/e2enew/route/route_with_management_fields_test.go

+				}`,
+			Headers:      map[string]string{"Authorization": base.GetToken()},
+			ExpectStatus: http.StatusOK,
+			Sleep:        time.Duration(2) * time.Second,


need to check body too

okay. Just for this one or all other PUTs too

Hi Nic, just to make sure, the response body contains dynamic information like timestamp, do we really have to check the body?
However, I have added ExpectCode as somewhat an alternative😄.
eg.

{ "code": 0, "message": "", "data": { "id": "r1", "create_time": 1617804834, "update_time": 1617804834, "uri": "/hello", "name": "new jack", "desc": "new desc", "upstream": { "nodes": { "172.16.238.20:1980": 1 }, "type": "roundrobin" }, "status": 1 }, "request_id": "22fbd110-3e00-4e2b-b267-528bdef67fd5" }

We can test the fixed part, example:

ExpectBody: []string{`"code":0`, `"name":"new jack"`},

Cool. Adding changes.

Now, all tests with PUT request have been updated.

Jaycean

LGTM

starsz · 2021-04-07T01:30:01Z

api/test/e2enew/route/route_with_management_fields_test.go

+		request.Header.Add("Authorization", base.GetToken())
+		resp, err := http.DefaultClient.Do(request)
+		if err != nil {
+			return


Maybe we can use gomega to assert the err is nil.

yeah sure. Good catch.

Well done.Thank you for your contribution.

api/test/e2enew/route/route_with_management_fields_test.go

juzhiyuan requested review from Jaycean and nic-chen April 2, 2021 10:10

Jaycean reviewed Apr 4, 2021

View reviewed changes

nic-chen reviewed Apr 5, 2021

View reviewed changes

nic-chen reviewed Apr 6, 2021

View reviewed changes

Jaycean requested review from starsz and tokers April 7, 2021 01:12

Jaycean approved these changes Apr 7, 2021

View reviewed changes

starsz reviewed Apr 7, 2021

View reviewed changes

api/test/e2enew/route/route_with_management_fields_test.go Outdated Show resolved Hide resolved

bisakhmondal added 5 commits April 7, 2021 23:10

feat: rewrite e2e test (route with management fields) using ginkgo

74d180f

chore: gitignore updated

cb951b6

feat: suggestions applied

5c6077b

feat: route not found body added

8a23e83

fix: suggestions applied from @nic-chen @starsz

347d1e7

bisakhmondal force-pushed the issue#1500_route_with_management_fields branch from bf776f4 to 347d1e7 Compare April 7, 2021 17:43

bisakhmondal requested review from starsz and nic-chen April 8, 2021 04:31

starsz approved these changes Apr 8, 2021

View reviewed changes

membphis approved these changes Apr 9, 2021

View reviewed changes

tokers approved these changes Apr 9, 2021

View reviewed changes

juzhiyuan merged commit 166137e into apache:master Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rewrite e2e test(test-e2e-route-with-management-fields) using ginkgo #1704

feat: rewrite e2e test(test-e2e-route-with-management-fields) using ginkgo #1704

bisakhmondal commented Apr 2, 2021

Jaycean Apr 4, 2021

bisakhmondal Apr 4, 2021

Jaycean Apr 4, 2021

bisakhmondal Apr 4, 2021

bisakhmondal commented Apr 4, 2021

juzhiyuan commented Apr 4, 2021

nic-chen Apr 5, 2021

bisakhmondal Apr 5, 2021

codecov-io commented Apr 5, 2021

nic-chen Apr 6, 2021

bisakhmondal Apr 6, 2021 •

edited

Loading

bisakhmondal Apr 7, 2021

nic-chen Apr 7, 2021

bisakhmondal Apr 7, 2021

bisakhmondal Apr 7, 2021

Jaycean left a comment

starsz Apr 7, 2021

bisakhmondal Apr 7, 2021

starsz Apr 7, 2021

feat: rewrite e2e test(test-e2e-route-with-management-fields) using ginkgo #1704

feat: rewrite e2e test(test-e2e-route-with-management-fields) using ginkgo #1704

Conversation

bisakhmondal commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisakhmondal commented Apr 4, 2021

juzhiyuan commented Apr 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 5, 2021

Codecov Report

Choose a reason for hiding this comment

bisakhmondal Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jaycean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisakhmondal Apr 6, 2021 •

edited

Loading