Copilot commented on code in PR #712: URL: https://github.com/apache/shenyu-dashboard/pull/712#discussion_r4229367243
########## src/routes/Plugin/Common/dictState.test.js: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import ConnectedSelector from "./Selector"; +import ConnectedRule from "./Rule"; + +let importModalProps; + Review Comment: `importModalProps` is module-scoped and used immediately after the click. If the modal component render/state update hasn’t occurred yet, `importModalProps` may still be `undefined`, causing a crash and potential test flakiness. Consider resetting `importModalProps` in a `beforeEach`, and after the click use `waitFor` (or another deterministic condition) to assert `importModalProps?.onOk` is available before calling it. ########## src/routes/Plugin/Common/dictState.test.js: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import ConnectedSelector from "./Selector"; +import ConnectedRule from "./Rule"; + +let importModalProps; + +jest.mock("../../../utils/IntlUtils", () => ({ + getIntlContent: (key) => key, +})); +jest.mock("./SelectorCopy", () => () => null); +jest.mock("./RuleCopy", () => () => null); +jest.mock("../Discovery/DiscoveryUpstreamTable.js", () => () => null); +jest.mock("../Discovery/DiscoveryImportModal", () => (props) => { + importModalProps = props; + return null; +}); Review Comment: `importModalProps` is module-scoped and used immediately after the click. If the modal component render/state update hasn’t occurred yet, `importModalProps` may still be `undefined`, causing a crash and potential test flakiness. Consider resetting `importModalProps` in a `beforeEach`, and after the click use `waitFor` (or another deterministic condition) to assert `importModalProps?.onOk` is available before calling it. ########## src/routes/Plugin/Common/Selector.js: ########## @@ -953,19 +955,21 @@ class AddModal extends Component { id: discoveryId = "", props = "{}", } = configData || {}; + const configPropsJson = JSON.parse(props); const formData = { selectedDiscoveryType: type, serverList, + ...configPropsJson, }; Review Comment: `JSON.parse(props)` is now executed before `setState`. If `props` contains invalid JSON, this will throw and prevent the modal from closing / state from updating. Since `props` comes from an import payload, it’s safer to wrap the parse in a `try/catch` and fall back to `{}` (and optionally surface a user-facing error) so the UI stays consistent even on malformed input. ########## src/routes/Plugin/Common/dictState.test.js: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import ConnectedSelector from "./Selector"; +import ConnectedRule from "./Rule"; + +let importModalProps; + +jest.mock("../../../utils/IntlUtils", () => ({ + getIntlContent: (key) => key, +})); +jest.mock("./SelectorCopy", () => () => null); +jest.mock("./RuleCopy", () => () => null); +jest.mock("../Discovery/DiscoveryUpstreamTable.js", () => () => null); +jest.mock("../Discovery/DiscoveryImportModal", () => (props) => { + importModalProps = props; + return null; +}); + +// Form.create() hoists the class unwrapped by dva connect(), so the modal can be +// mounted with a real form but without a store. +const unwrap = (component) => Form.create()(component.WrappedComponent); +const Selector = unwrap(ConnectedSelector); +const Rule = unwrap(ConnectedRule); + +const matchModeDics = [ + { dictValue: "0", dictName: "and" }, + { dictValue: "1", dictName: "or" }, +]; + +// Hold dictionary requests until the test resolves them, like a slow backend. +const createDispatch = () => { + const pending = {}; + const dispatch = jest.fn(({ type, payload }) => { + if (type === "shenyuDict/fetchByType") { + pending[payload.type] = payload.callBack; + } + }); + const resolve = (type, dics) => act(() => pending[type](dics)); + return { dispatch, resolve }; +}; + +const matchModeSelection = () => + document.querySelector("#matchMode .ant-select-selection-selected-value") + .textContent; Review Comment: This selector relies on Ant Design’s internal DOM structure/classes (`.ant-select-selection-selected-value`), which can change across AntD versions and make tests brittle. Prefer Testing Library queries (by role/label/text) or target a stable attribute (e.g., `data-testid`) in the component if available. ########## src/routes/Plugin/Common/dictState.test.js: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import ConnectedSelector from "./Selector"; +import ConnectedRule from "./Rule"; + +let importModalProps; + +jest.mock("../../../utils/IntlUtils", () => ({ + getIntlContent: (key) => key, +})); +jest.mock("./SelectorCopy", () => () => null); +jest.mock("./RuleCopy", () => () => null); +jest.mock("../Discovery/DiscoveryUpstreamTable.js", () => () => null); +jest.mock("../Discovery/DiscoveryImportModal", () => (props) => { + importModalProps = props; + return null; +}); + +// Form.create() hoists the class unwrapped by dva connect(), so the modal can be +// mounted with a real form but without a store. +const unwrap = (component) => Form.create()(component.WrappedComponent); +const Selector = unwrap(ConnectedSelector); +const Rule = unwrap(ConnectedRule); + +const matchModeDics = [ + { dictValue: "0", dictName: "and" }, + { dictValue: "1", dictName: "or" }, +]; + +// Hold dictionary requests until the test resolves them, like a slow backend. +const createDispatch = () => { + const pending = {}; + const dispatch = jest.fn(({ type, payload }) => { + if (type === "shenyuDict/fetchByType") { + pending[payload.type] = payload.callBack; + } + }); + const resolve = (type, dics) => act(() => pending[type](dics)); Review Comment: `resolve()` will throw a low-signal error (`pending[type] is not a function`) if the callback wasn’t registered (e.g., unexpected dict type or timing). To make failures easier to diagnose, add an explicit guard that throws a clear error when `pending[type]` is missing (or return early with a descriptive message). ########## src/routes/Plugin/Common/dictState.test.js: ########## @@ -0,0 +1,171 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React from "react"; +import { act, fireEvent, render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import ConnectedSelector from "./Selector"; +import ConnectedRule from "./Rule"; + +let importModalProps; + +jest.mock("../../../utils/IntlUtils", () => ({ + getIntlContent: (key) => key, +})); +jest.mock("./SelectorCopy", () => () => null); +jest.mock("./RuleCopy", () => () => null); +jest.mock("../Discovery/DiscoveryUpstreamTable.js", () => () => null); +jest.mock("../Discovery/DiscoveryImportModal", () => (props) => { + importModalProps = props; + return null; +}); + +// Form.create() hoists the class unwrapped by dva connect(), so the modal can be +// mounted with a real form but without a store. +const unwrap = (component) => Form.create()(component.WrappedComponent); +const Selector = unwrap(ConnectedSelector); +const Rule = unwrap(ConnectedRule); + +const matchModeDics = [ + { dictValue: "0", dictName: "and" }, + { dictValue: "1", dictName: "or" }, +]; + +// Hold dictionary requests until the test resolves them, like a slow backend. +const createDispatch = () => { + const pending = {}; + const dispatch = jest.fn(({ type, payload }) => { + if (type === "shenyuDict/fetchByType") { + pending[payload.type] = payload.callBack; + } + }); + const resolve = (type, dics) => act(() => pending[type](dics)); + return { dispatch, resolve }; +}; + +const matchModeSelection = () => + document.querySelector("#matchMode .ant-select-selection-selected-value") + .textContent; + +describe("dictionary responses that arrive after mount", () => { + it("re-render Selector options", () => { + const { dispatch, resolve } = createDispatch(); + render( + <Selector + dispatch={dispatch} + platform={{ selectorTypeEnums: [] }} + discovery={{ typeEnums: ["local", "zookeeper", "nacos"] }} + pluginId="5" + matchMode={0} + type={1} + onCancel={jest.fn()} + handleOk={jest.fn()} + />, + ); + + expect(matchModeSelection()).toBe("0"); + resolve("matchMode", matchModeDics); + expect(matchModeSelection()).toBe("and"); + }); + + it("re-render Rule options", () => { + const { dispatch, resolve } = createDispatch(); + render( + <Rule + dispatch={dispatch} + pluginId="5" + pluginName="divide" + matchMode={0} + onCancel={jest.fn()} + handleOk={jest.fn()} + />, + ); + + expect(matchModeSelection()).toBe("0"); + resolve("matchMode", matchModeDics); + expect(matchModeSelection()).toBe("and"); + }); +}); +describe("cached dictionaries", () => { + it("are applied when Rule receives them synchronously", () => { + const dispatch = ({ type, payload }) => { + if (type === "shenyuDict/fetchByType" && payload.type === "matchMode") { + payload.callBack(matchModeDics); + } + }; + render( + <Rule + dispatch={dispatch} + pluginId="5" + pluginName="divide" + matchMode={0} + onCancel={jest.fn()} + handleOk={jest.fn()} + />, + ); + + expect(matchModeSelection()).toBe("and"); + }); +}); + +describe("Selector discovery import", () => { + it("renders the imported properties and their values", () => { + const { dispatch, resolve } = createDispatch(); + render( + <Selector + dispatch={dispatch} + platform={{ selectorTypeEnums: [] }} + discovery={{ typeEnums: ["local", "zookeeper", "nacos"] }} + pluginId="5" + type={1} + isDiscovery + isAdd + discoveryConfig={{ discoveryType: "local" }} + onCancel={jest.fn()} + handleOk={jest.fn()} + />, + ); + resolve("discoveryMode", [ + { dictName: "zookeeper", dictValue: '{"sessionTimeout":"1000"}' }, + ]); + fireEvent.click( + screen.getByText("SHENYU.DISCOVERY.SELECTOR.CONFIG.DISCOVERY"), + ); + const importConfig = (props) => + act(() => + importModalProps.onOk({ + type: "nacos", + serverList: "127.0.0.1:8848", + id: "discovery-1", + props, + }), + ); Review Comment: `importModalProps` is module-scoped and used immediately after the click. If the modal component render/state update hasn’t occurred yet, `importModalProps` may still be `undefined`, causing a crash and potential test flakiness. Consider resetting `importModalProps` in a `beforeEach`, and after the click use `waitFor` (or another deterministic condition) to assert `importModalProps?.onOk` is available before calling it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
