Skip to content

Conversation

SkyeYoung
Copy link
Member

@SkyeYoung SkyeYoung commented Jun 10, 2025

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
  • Test cases

What changes will this PR take into?

Depend on apache/apisix#12291, #3111, #3112, #3117

Meanwhile, some essential code that needed fixing has been fixed.

Paths:

e2e/tests/services.stream_routes.list.spec.ts:

  1. should only show stream_routes with service_id
    1. under services/stream_routes, only show stream_routes with current service_id
    2. under services/stream_routes, not show stream_routes with other service_id
    3. under stream_routes, show all stream_routes. (Since the api does not yet accept multiple service IDs, no filtering will be done on the routes list page.)
  2. should display routes list under service
    1. check if every components works

e2e/tests/services.stream_routes.crud.spec.ts:

  1. should CRUD route under stream_routes with required fields
  2. I think all stream_routes resource tests should be tested in stream_routes.crud-*.spec.ts, the CRUD tests included in this PR reduce the repetition of existing paths.

@SkyeYoung SkyeYoung marked this pull request as ready for review June 13, 2025 03:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds test coverage for stream routes under services and introduces related schema, UI, form, and API enhancements to support these tests.

  • Stream route schema updates to make conf and logger optional
  • Integration of produceRmUpstreamWhenHas into add/edit form pipelines
  • Addition of deleteAllStreamRoutes helper and comprehensive E2E tests for listing and CRUD

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/types/schema/apisix/stream_routes.ts Made conf and logger optional in StreamRouteProtocol schema
src/routes/stream_routes/index.tsx Updated list columns to display server_addr and server_port
src/routes/stream_routes/detail.$id.tsx Applied produceRmUpstreamWhenHas in update mutation pipeline
src/routes/stream_routes/add.tsx Applied produceRmUpstreamWhenHas in creation mutation pipeline
src/components/form/JsonInput.tsx Extended JSON input with objValue support and omitted prop
src/components/form-slice/FormPartStreamRoute/index.tsx Provided objValue to JSON inputs for protocol fields
src/apis/stream_routes.ts Added deleteAllStreamRoutes helper
e2e/tests/services.stream_routes.list.spec.ts E2E tests for service-scoped stream routes listing
e2e/tests/services.stream_routes.crud.spec.ts E2E tests for service-scoped stream routes CRUD
e2e/pom/services.ts POM updates for navigating and interacting with stream routes
Comments suppressed due to low confidence (3)

src/routes/stream_routes/detail.$id.tsx:42

  • [nitpick] The function name produceRmUpstreamWhenHas is cryptic; consider renaming to something more descriptive like removeUpstreamIfServiceIdPresent for clarity.
import { produceRmUpstreamWhenHas } from '@/utils/form-producer';

src/apis/stream_routes.ts:67

  • There aren't any unit tests covering deleteAllStreamRoutes; adding tests to confirm it correctly paginates and deletes all items would catch regressions early.
export const deleteAllStreamRoutes = async (req: AxiosInstance) => {

src/components/form/JsonInput.tsx:18

  • [nitpick] Introducing a new dependency just for omit may increase bundle size and cognitive load; consider using built-in object destructuring or a shared utility (e.g., lodash.omit) to maintain consistency.
import { omit } from 'rambdax';

@SkyeYoung SkyeYoung marked this pull request as ready for review June 16, 2025 01:53
@SkyeYoung SkyeYoung requested a review from juzhiyuan June 16, 2025 01:56
@SkyeYoung SkyeYoung merged commit 70712bd into apache:master Jun 16, 2025
5 checks passed
@SkyeYoung SkyeYoung deleted the young/test/stream_routes-in-services branch June 16, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants