utafrali commented on code in PR #4073: URL: https://github.com/apache/rocketmq-dashboard/pull/4073#discussion_r3949991342
########## PR_BODY.md: ########## @@ -0,0 +1,14 @@ +Fixes #4028 Review Comment: `PR_BODY.md` should not be committed to the repository. This file is pull-request metadata and has no place in the source tree. Please remove it from the commit. ########## web/src/pages/ops/__tests__/alertRulePayload.test.ts: ########## @@ -0,0 +1,49 @@ +/* + * 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 { describe, expect, it } from 'vitest'; +import { + attachThresholdUnit, + normalizeDuration, + normalizeMetric, + thresholdUnits, +} from '../alertRulePayload'; + +describe('alert rule payload normalization', () => { + it('maps legacy Chinese metric labels to Prometheus metric names', () => { + expect(normalizeMetric('磁盘使用率')).toBe('rocketmq_disk_use_ratio'); + expect(normalizeMetric('rocketmq_tps')).toBe('rocketmq_tps'); + }); + + it('maps legacy Chinese duration labels to Prometheus durations', () => { Review Comment: The description "maps legacy Chinese duration labels to Prometheus durations" doesn't capture line 35, which tests that an unmapped input (`'45分钟'`) passes through unchanged. A reader skimming the test names won't know the fallback behavior is locked here. Consider renaming to something like `'maps known Chinese duration labels and passes unknown ones through unchanged'`. ########## web/src/pages/ops/__tests__/alertRulePayload.test.ts: ########## @@ -0,0 +1,49 @@ +/* + * 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 { describe, expect, it } from 'vitest'; +import { + attachThresholdUnit, + normalizeDuration, + normalizeMetric, + thresholdUnits, +} from '../alertRulePayload'; + +describe('alert rule payload normalization', () => { + it('maps legacy Chinese metric labels to Prometheus metric names', () => { + expect(normalizeMetric('磁盘使用率')).toBe('rocketmq_disk_use_ratio'); + expect(normalizeMetric('rocketmq_tps')).toBe('rocketmq_tps'); + }); + + it('maps legacy Chinese duration labels to Prometheus durations', () => { + expect(normalizeDuration('1分钟')).toBe('1m'); + expect(normalizeDuration('30分钟')).toBe('30m'); + expect(normalizeDuration('45分钟')).toBe('45分钟'); + }); + + it('attaches the metric threshold unit after normalization', () => { Review Comment: This `attachThresholdUnit` test with a legacy Chinese metric and duration (`磁盘使用率` / `1分钟`) is almost identical to an existing test in `alerts.test.ts` (lines 44-53), which already covers `attachThresholdUnit` with `磁盘使用率` / `5分钟`. The two cases exercise the exact same code paths; only the threshold value and duration string differ, so this block doesn't add regression protection beyond what's already locked. Consider removing this `it` block and letting the standalone `normalizeMetric` / `normalizeDuration` tests in this file carry their own weight. ########## web/src/pages/ops/__tests__/alertRulePayload.test.ts: ########## @@ -0,0 +1,49 @@ +/* + * 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 { describe, expect, it } from 'vitest'; +import { + attachThresholdUnit, + normalizeDuration, + normalizeMetric, + thresholdUnits, +} from '../alertRulePayload'; + +describe('alert rule payload normalization', () => { + it('maps legacy Chinese metric labels to Prometheus metric names', () => { Review Comment: Only one of the five legacy metric mappings is tested directly via `normalizeMetric`. The PR description says it locks "the mapping of legacy Chinese metric/duration labels," but `消费堆积量`, `TPS 异常`, `Broker 离线`, and `Proxy 连接数` have no direct assertions. A single accidental rename in `legacyMetricValues` for those keys would pass this suite. Add one assertion per remaining mapping to fully lock the table. ########## web/src/pages/ops/__tests__/alertRulePayload.test.ts: ########## @@ -0,0 +1,49 @@ +/* + * 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 { describe, expect, it } from 'vitest'; +import { + attachThresholdUnit, + normalizeDuration, + normalizeMetric, + thresholdUnits, +} from '../alertRulePayload'; + +describe('alert rule payload normalization', () => { + it('maps legacy Chinese metric labels to Prometheus metric names', () => { + expect(normalizeMetric('磁盘使用率')).toBe('rocketmq_disk_use_ratio'); + expect(normalizeMetric('rocketmq_tps')).toBe('rocketmq_tps'); + }); + + it('maps legacy Chinese duration labels to Prometheus durations', () => { + expect(normalizeDuration('1分钟')).toBe('1m'); + expect(normalizeDuration('30分钟')).toBe('30m'); + expect(normalizeDuration('45分钟')).toBe('45分钟'); + }); + + it('attaches the metric threshold unit after normalization', () => { + expect( + attachThresholdUnit({ metric: '磁盘使用率', duration: '1分钟', threshold: 80 }), + ).toEqual({ + metric: 'rocketmq_disk_use_ratio', + duration: '1m', + threshold: 80, + thresholdUnit: '%', + }); + expect(thresholdUnits['rocketmq_consumer_lag_messages']).toBe('条'); Review Comment: `expect(thresholdUnits['rocketmq_consumer_lag_messages']).toBe('条')` belongs in its own `it` block, not appended to the `attachThresholdUnit` test. Right now it reads as a stray assertion that verifies a map entry but has nothing to do with the function under test. Move it to something like `it('exports threshold units for all known metrics', () => { ... })` and cover the remaining four entries (`rocketmq_disk_use_ratio`, `rocketmq_tps`, `rocketmq_broker_offline`, `rocketmq_proxy_connections`) there too. -- 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]
