sadpandajoe commented on code in PR #44201: URL: https://github.com/apache/superset/pull/44201#discussion_r4129965675
########## superset-frontend/custom-lint-rules/theme-colors/no-literal-colors.test.ts: ########## @@ -0,0 +1,66 @@ +/** + * 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 { RuleTester } from 'oxlint/plugins-dev'; +import { describe, it } from 'node:test'; +import plugin from '.'; + +RuleTester.describe = describe; +RuleTester.it = it; + +const ruleTester = new RuleTester(); +const rule = plugin.rules['no-literal-colors']; + +const errors: Array<{ message: string }> = [ + { + message: + 'Theme color variables are preferred over rgb(a)/hex/literal colors', + }, +]; + +ruleTester.run('no-literal-colors', rule, { + valid: [ + 'const styles = { color: theme.colorText, background: theme.colorBg };', + "const colors = { red: 'not a property value' };", + "const color = 'red';", + 'styled.div`color: ${theme.colorText};`', + ], + invalid: [ Review Comment: Every `invalid` case here reports at a distinct source location, so none re-triggers a location a prior case already reported — the same-location de-dupe this rule's `warned` array exists for is never exercised. Hoisting `warned` back to module scope (outside `create()`) still passes all nine cases unchanged; only a case that repeats an already-flagged location (e.g. a second case like `{ color: 'tan' }` at the same position as the existing `{ color: 'red' }` case) fails against that regression and passes against the current per-call reset. Worth adding one such case to lock in the fix? ########## superset-frontend/scripts/internal/oxlint-metrics-uploader.js: ########## @@ -130,102 +163,52 @@ async function runOxlintAndProcess() { ); const results = JSON.parse(oxlintOutput); - - // Process OXC JSON output - const metricsByRule = {}; - let occurrencesData = []; - - // OXC JSON format has diagnostics array - if (results.diagnostics && Array.isArray(results.diagnostics)) { - results.diagnostics.forEach(diagnostic => { - const ruleId = parseRuleId(diagnostic.code); - - const file = diagnostic.filename || 'unknown'; - const line = diagnostic.labels?.[0]?.span?.line || 0; - const column = diagnostic.labels?.[0]?.span?.column || 0; - const message = diagnostic.message || ''; - - const ruleData = metricsByRule[ruleId] || { count: 0 }; - ruleData.count += 1; - metricsByRule[ruleId] = ruleData; - - occurrencesData.push({ - rule: ruleId, - message, - file, - line, - column, - ts: DATETIME, - }); - }); - } - console.log( `OXC found ${results.diagnostics?.length || 0} issues across ${results.number_of_files} files`, ); + const { metricsByRule, occurrencesData } = parseOxlintResult(results); + + // Also run Oxlint for custom rules and merge results + console.log('Running Oxlint for custom rules...'); + // Run ESLint and capture output directly. + // Flat config (oxlint.custom-lint-rules.mts) is explicitly selected via --config + const oxlintCustomRuleOutput = execSync( + 'npx oxlint --config oxlint.custom-lint-rules.mts --format json src', + { + encoding: 'utf8', + maxBuffer: 50 * 1024 * 1024, // 50MB buffer for large outputs + stdio: ['pipe', 'pipe', 'ignore'], // Ignore stderr + }, + ); - // Also run minimal ESLint for custom rules and merge results - console.log('Running minimal ESLint for custom rules...'); - let eslintOutput = '[]'; - try { - // Run ESLint and capture output directly. - // Flat config (eslint.config.minimal.js) is explicitly selected via - // --config; ESLint v9+/v10 no longer support eslintrc or --no-eslintrc. - eslintOutput = execSync( - 'npx eslint --config eslint.config.minimal.js --no-inline-config --format json src', - { - encoding: 'utf8', - maxBuffer: 50 * 1024 * 1024, - stdio: ['pipe', 'pipe', 'ignore'], // Ignore stderr - }, - ); - } catch (e) { - // ESLint exits with non-zero when it finds issues, capture the stdout - if (e.stdout) { - eslintOutput = e.stdout.toString(); - } - } - - // Parse minimal ESLint output - try { - const eslintResults = JSON.parse(eslintOutput); - - eslintResults.forEach(result => { - result.messages.forEach(({ ruleId, line, column, message }) => { - const ruleData = metricsByRule[ruleId] || { count: 0 }; - ruleData.count += 1; - metricsByRule[ruleId] = ruleData; - - occurrencesData.push({ - rule: ruleId, - message, - file: result.filePath, - line, - column, - ts: DATETIME, - }); - }); - }); - - console.log( - `ESLint found ${eslintResults.reduce((sum, r) => sum + r.messages.length, 0)} custom rule violations`, - ); - } catch (e) { - console.log('No ESLint issues found or parsing error:', e.message); - } + // Parse Oxlint output for custom rules + const oxlintCustomRuleResults = JSON.parse(oxlintCustomRuleOutput); + console.log( + `OXC found ${oxlintCustomRuleResults.diagnostics?.length || 0} issues across ${oxlintCustomRuleResults.number_of_files} files for custom rules`, + ); + const { + metricsByRule: metricsByCustomRule, + occurrencesData: customRuleOccurrencesData, + } = parseOxlintResult(oxlintCustomRuleResults); + + const mergedMetricsByRule = { ...metricsByRule, ...metricsByCustomRule }; Review Comment: Reproduced this against the current head: adding a plain `debugger;` statement under `src/` makes both the standard pass (`oxlint.json`) and this custom-rule pass (`oxlint.custom-lint-rules.mts`) report `eslint(no-debugger)`, since the custom config only excludes four specific built-in rules rather than disabling built-in categories outright. The uploader's `{...metricsByRule, ...metricsByCustomRule}` spread then overwrites the first pass's count for that rule with the second pass's, while the occurrence arrays concatenate both — so the aggregated metrics sheet undercounts while the backlog sheet gets a duplicate row for the same violation. Any built-in rule outside the current four-item exclusion list reproduces it, and there's no test guarding the merge logic either. Would scoping the custom pass's accepted diagnostics by rule-ID prefix (`theme-colors/`, `icons/`, `i18n-strings/`) instead of by an exclusion denylist close this for good? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
