fix: supports search by name for service options when add router. #2066

TkClark · 2021-08-12T08:19:29Z

Web ui supports search by name for service options when add router.
fix: When use the dashboard ui the priority of the route cann't be set if the upstream is not set when the route is set.

2. fixed the bug where the priority of the route cann't be set if the upstream is not set when the route is set.

netlify · 2021-08-12T08:19:35Z

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: 5e59e0a

🔍 Inspect the deploy log: https://app.netlify.com/sites/apisix-dashboard/deploys/6136e5f672b2b800088da576

😎 Browse the preview: https://deploy-preview-2066--apisix-dashboard.netlify.app/

codecov-commenter · 2021-08-23T00:52:34Z

Codecov Report

Merging #2066 (80861ae) into master (79012fd) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2066      +/-   ##
==========================================
+ Coverage   67.48%   67.52%   +0.04%     
==========================================
  Files         126      126              
  Lines        3294     3295       +1     
  Branches      802      802              
==========================================
+ Hits         2223     2225       +2     
+ Misses       1071     1070       -1

Flag	Coverage Δ
frontend-e2e-test	`67.52% <100.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
web/src/pages/Route/transform.ts	`78.68% <ø> (ø)`
web/src/pages/Route/components/Step1/MetaView.tsx	`100.00% <100.00%> (ø)`
...omponents/Upstream/components/UpstreamSelector.tsx	`100.00% <0.00%> (+20.00%)`	⬆️

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 79012fd...80861ae. Read the comment docs.

liuxiran · 2021-08-23T05:40:24Z

Hi @TkClark , would you please add test case for your bug fix?

for example, create a Route with service and priority=1, and then check in the configure view to see if the priority field is 1

thanks very much.

and I would be better to change your commit message to some thing like:

fix: xxxxx.

thanks very much.

TkClark · 2021-09-07T03:55:13Z

Hi @TkClark , would you please add test case for your bug fix?

for example, create a Route with service and priority=1, and then check in the configure view to see if the priority field is 1

thanks very much.

and I would be better to change your commit message to some thing like:

fix: xxxxx.

thanks very much.

just fixed the frontend bugs. And i don't see the test of react frontend

liuxiran · 2021-09-08T05:08:44Z

Thanks for your feedback @TkClark , then let's turn to the test cases. Here is our fe e2e test develop guide, hope this can help you to write and debug your test cases.

Actually your pr contains two part of things:

feat: supports search by name for service options when add router

you can choose to start a new test file to add your test cases for this feature.

create a service, you can refer this one:

apisix-dashboard/web/cypress/integration/service/create-edit-delete-service.spec.js

Line 60 in 98fe38a

it('should create service', function () {
create a route, you can refer this one:

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 88 in 98fe38a

it.only('should not create route with name above 100 characters', function () {

during create a route, you can add some test code to cover you feature:
file in the service field(part of your created service name), to check if there is a service item listed.

fix: would not set `priority` when create route

you can add some test codes to this file: https://github.com/apache/apisix-dashboard/blob/master/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

when create a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 102 in 98fe38a

it('should create route', function () {

try to file in the priority field together with other fields.
when view a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 210 in 98fe38a

it('should view the route', function () {

try to check if the priority field existed
when edit a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 230 in 98fe38a

it('should edit the route', function () {

try to check if the priority has the right value, you can refer this one cy.get({priority}).should('have.value', xxxx)

fix: would not set priority when create route add test cases for this feature.

TkClark · 2021-09-10T09:54:32Z

Thanks for your feedback @TkClark , then let's turn to the test cases. Here is our fe e2e test develop guide, hope this can help you to write and debug your test cases.

Actually your pr contains two part of things:

feat: supports search by name for service options when add router

you can choose to start a new test file to add your test cases for this feature.

create a service, you can refer this one:

apisix-dashboard/web/cypress/integration/service/create-edit-delete-service.spec.js

Line 60 in 98fe38a

it('should create service', function () {

create a route, you can refer this one:

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 88 in 98fe38a

it.only('should not create route with name above 100 characters', function () {

during create a route, you can add some test code to cover you feature:
file in the service field(part of your created service name), to check if there is a service item listed.

fix: would not set priority when create route

you can add some test codes to this file: https://github.com/apache/apisix-dashboard/blob/master/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

when create a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 102 in 98fe38a

it('should create route', function () {

try to file in the priority field together with other fields.

when view a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 210 in 98fe38a

it('should view the route', function () {

try to check if the priority field existed

when edit a route in

apisix-dashboard/web/cypress/integration/route/create-edit-duplicate-delete-route.spec.js

Line 230 in 98fe38a

it('should edit the route', function () {

try to check if the priority has the right value, you can refer this one cy.get({priority}).should('have.value', xxxx)

OK.

liuxiran · 2021-09-13T09:26:54Z

web/cypress/integration/route/create-route-with-search-service-and-set-priority.spec.js

+    cy.contains('View').click();
+    cy.get(selector.drawer).should('be.visible');
+    cy.get(selector.monacoScroll).within(() => {
+      cy.contains('priority').should('exist');


LGTM

It would be better to check whether service_id exist

i was add check whether service_id exist for the test case

liuxiran · 2021-09-13T09:27:53Z

also cc @Baoyuantop @iamayushdas to have a look, thanks

liuxiran

LGMT

liuxiran · 2021-09-16T01:44:45Z

After checking the video log, we can found that the E2E failed caused by

we need to find a better way to fix this unstable test case.

video log here:
https://user-images.githubusercontent.com/2561857/133535785-4eae0421-9b8c-43e7-b667-9a3bc879c8ad.mp4

liuxiran · 2021-09-16T01:49:52Z

re-run FE E2E test case

1. supports search by name for service options when add router.

1c1625b

2. fixed the bug where the priority of the route cann't be set if the upstream is not set when the route is set.

TkClark changed the title ~~supports search by name for service options when add router. fix bug~~ fix: supports search by name for service options when add router. Sep 7, 2021

Merge remote-tracking branch 'origin/master' into dev/zc

5e59e0a

liuxiran added this to the 2.9 milestone Sep 8, 2021

TkClark added 2 commits September 10, 2021 11:22

Merge remote-tracking branch 'origin/master' into dev/zc

f17e0f1

feat: supports search by name for service options when add router

388bd1d

fix: would not set priority when create route add test cases for this feature.

liuxiran requested review from juzhiyuan, LiteSun and bzp2010 September 13, 2021 09:21

liuxiran reviewed Sep 13, 2021

View reviewed changes

fix: add check whether service_id exist for the test case

80861ae

liuxiran approved these changes Sep 13, 2021

View reviewed changes

Baoyuantop approved these changes Sep 14, 2021

View reviewed changes

liuxiran mentioned this pull request Sep 16, 2021

test: unstable test case search-route #2140

Closed

bzp2010 approved these changes Sep 19, 2021

View reviewed changes

iamayushdas approved these changes Sep 19, 2021

View reviewed changes

juzhiyuan approved these changes Sep 19, 2021

View reviewed changes

juzhiyuan merged commit f9f4b36 into apache:master Sep 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: supports search by name for service options when add router. #2066

fix: supports search by name for service options when add router. #2066

TkClark commented Aug 12, 2021 •

edited

Loading

netlify bot commented Aug 12, 2021 •

edited

Loading

codecov-commenter commented Aug 23, 2021 •

edited

Loading

liuxiran commented Aug 23, 2021 •

edited

Loading

TkClark commented Sep 7, 2021

liuxiran commented Sep 8, 2021

TkClark commented Sep 10, 2021

feat: supports search by name for service options when add router

fix: would not set `priority` when create route

liuxiran Sep 13, 2021

TkClark Sep 13, 2021

liuxiran commented Sep 13, 2021

liuxiran left a comment

liuxiran commented Sep 16, 2021

liuxiran commented Sep 16, 2021

fix: supports search by name for service options when add router. #2066

fix: supports search by name for service options when add router. #2066

Conversation

TkClark commented Aug 12, 2021 • edited Loading

netlify bot commented Aug 12, 2021 • edited Loading

codecov-commenter commented Aug 23, 2021 • edited Loading

Codecov Report

liuxiran commented Aug 23, 2021 • edited Loading

TkClark commented Sep 7, 2021

liuxiran commented Sep 8, 2021

feat: supports search by name for service options when add router

fix: would not set priority when create route

TkClark commented Sep 10, 2021

feat: supports search by name for service options when add router

fix: would not set priority when create route

liuxiran Sep 13, 2021

Choose a reason for hiding this comment

TkClark Sep 13, 2021

Choose a reason for hiding this comment

liuxiran commented Sep 13, 2021

liuxiran left a comment

Choose a reason for hiding this comment

liuxiran commented Sep 16, 2021

liuxiran commented Sep 16, 2021

TkClark commented Aug 12, 2021 •

edited

Loading

netlify bot commented Aug 12, 2021 •

edited

Loading

codecov-commenter commented Aug 23, 2021 •

edited

Loading

liuxiran commented Aug 23, 2021 •

edited

Loading

fix: would not set `priority` when create route

fix: would not set `priority` when create route