You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2026/03/09 14:06:58 UTC
[PR] fix(tests): achieve 100% TypeScript coverage for core packages [superset]
michael-s-molina opened a new pull request, #38518:
URL: https://github.com/apache/superset/pull/38518
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
The Codecov `core-packages-ts` check was failing at 99.24% against a 100% target. This PR adds missing unit tests and `/* istanbul ignore next */` directives to untestable branches so the check passes.
**Root cause:** A previous refactor (`357e35dc62`) moved `Translator.ts` and `TranslatorSingleton.ts` into `superset-core` as new files without accompanying tests, and modified theme utilities without covering the new code paths. Additionally, `getXAxis.ts` in `superset-ui-core` had two entirely untested functions.
### TESTING INSTRUCTIONS
```bash
# Run superset-core tests with coverage
cd superset-frontend
npx jest --coverage --coverageReporters=text \
--collectCoverageFrom='packages/superset-core/src/**/*.ts' \
'packages/superset-core'
# Run superset-ui-core tests with coverage
npx jest --coverage --coverageReporters=text \
--collectCoverageFrom='packages/superset-ui-core/src/**/*.ts' \
'packages/superset-ui-core'
```
Both should report **100% statements / branches / functions / lines** for `.ts` files.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tests): achieve 100% TypeScript coverage for core packages [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #38518:
URL: https://github.com/apache/superset/pull/38518
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tests): achieve 100% TypeScript coverage for core packages [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38518:
URL: https://github.com/apache/superset/pull/38518#discussion_r2905747365
##########
superset-frontend/packages/superset-core/src/translation/TranslatorSingleton.test.ts:
##########
@@ -0,0 +1,112 @@
+/**
+ * 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.
+ */
+
+// Each test uses jest.isolateModules to get a fresh module state
+// (isConfigured = false, singleton = undefined) independent of other tests.
+
+test('t() warns and creates a default translator when called before configure', () => {
+ jest.isolateModules(() => {
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { t } = require('./TranslatorSingleton');
+ const result = t('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'You should call configure(...) before calling other methods',
+ );
+ expect(result).toBe('hello');
+ consoleSpy.mockRestore();
+ });
+});
+
+test('configure sets up the singleton and t() works without warnings', () => {
+ jest.isolateModules(() => {
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { configure, t } = require('./TranslatorSingleton');
+ configure();
+ const result = t('hello');
+ expect(consoleSpy).not.toHaveBeenCalled();
+ expect(result).toBe('hello');
+ consoleSpy.mockRestore();
+ });
+});
+
+test('resetTranslation resets the configured singleton', () => {
+ jest.isolateModules(() => {
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { configure, resetTranslation, t } = require('./TranslatorSingleton');
+ configure();
+ resetTranslation();
+ // After reset, calling t() should warn again
+ t('hello');
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'You should call configure(...) before calling other methods',
+ );
+ consoleSpy.mockRestore();
+ });
+});
+
+test('addTranslation adds a translation via the singleton', () => {
+ jest.isolateModules(() => {
+ const { configure, addTranslation, t } = require('./TranslatorSingleton');
+ configure();
+ addTranslation('greeting', ['Hello!']);
+ expect(t('greeting')).toBe('Hello!');
+ });
+});
+
+test('addTranslations adds multiple translations via the singleton', () => {
+ jest.isolateModules(() => {
+ const { configure, addTranslations, t } = require('./TranslatorSingleton');
+ configure();
+ addTranslations({ farewell: ['Goodbye!'] });
+ expect(t('farewell')).toBe('Goodbye!');
+ });
+});
+
+test('addLocaleData adds locale translations via the singleton', () => {
+ jest.isolateModules(() => {
+ const { configure, addLocaleData, t } = require('./TranslatorSingleton');
+ configure();
+ addLocaleData({ en: { locale_key: ['locale value'] } });
+ expect(t('locale_key')).toBe('locale value');
+ });
+});
+
+test('tn() calls translateWithNumber on the singleton', () => {
+ jest.isolateModules(() => {
+ const { configure, tn } = require('./TranslatorSingleton');
+ configure();
+ const result = tn('item', 1);
+ expect(typeof result).toBe('string');
+ });
+});
+
+test('resetTranslation does nothing when not yet configured', () => {
+ jest.isolateModules(() => {
+ const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
+ const { resetTranslation, t } = require('./TranslatorSingleton');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Replace require() with ES6 import statements</b></div>
<div id="fix">
The code uses `require()` style imports within `jest.isolateModules()` blocks, which violates the `@typescript-eslint/no-require-imports` rule. Replace all 8 instances with ES6 `import` statements to comply with the linting standards.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
// Each test uses jest.isolateModules to get a fresh module state
// (isConfigured = false, singleton = undefined) independent of other tests.
test('t() warns and creates a default translator when called before configure', () => {
jest.isolateModules(() => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const { t } = await import('./TranslatorSingleton');
const result = t('hello');
expect(consoleSpy).toHaveBeenCalledWith(
'You should call configure(...) before calling other methods',
);
expect(result).toBe('hello');
consoleSpy.mockRestore();
});
});
test('configure sets up the singleton and t() works without warnings', () => {
jest.isolateModules(() => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const { configure, t } = await import('./TranslatorSingleton');
configure();
const result = t('hello');
expect(consoleSpy).not.toHaveBeenCalled();
expect(result).toBe('hello');
consoleSpy.mockRestore();
});
});
test('resetTranslation resets the configured singleton', () => {
jest.isolateModules(() => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const { configure, resetTranslation, t } = await import('./TranslatorSingleton');
configure();
resetTranslation();
// After reset, calling t() should warn again
t('hello');
expect(consoleSpy).toHaveBeenCalledWith(
'You should call configure(...) before calling other methods',
);
consoleSpy.mockRestore();
});
});
test('addTranslation adds a translation via the singleton', () => {
jest.isolateModules(() => {
const { configure, addTranslation, t } = await import('./TranslatorSingleton');
configure();
addTranslation('greeting', ['Hello!']);
expect(t('greeting')).toBe('Hello!');
});
});
test('addTranslations adds multiple translations via the singleton', () => {
jest.isolateModules(() => {
const { configure, addTranslations, t } = await import('./TranslatorSingleton');
configure();
addTranslations({ farewell: ['Goodbye!'] });
expect(t('farewell')).toBe('Goodbye!');
});
});
test('addLocaleData adds locale translations via the singleton', () => {
jest.isolateModules(() => {
const { configure, addLocaleData, t } = await import('./TranslatorSingleton');
configure();
addLocaleData({ en: { locale_key: ['locale value'] } });
expect(t('locale_key')).toBe('locale value');
});
});
test('tn() calls translateWithNumber on the singleton', () => {
jest.isolateModules(() => {
const { configure, tn } = await import('./TranslatorSingleton');
configure();
const result = tn('item', 1);
expect(typeof result).toBe('string');
});
});
test('resetTranslation does nothing when not yet configured', () => {
jest.isolateModules(() => {
const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
const { resetTranslation, t } = await import('./TranslatorSingleton');
````
</div>
</details>
</div>
<small><i>Code Review Run #f50da4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-core/src/translation/Translator.test.ts:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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 Translator from './Translator';
+import logging from '../utils/logging';
+
+let warnSpy: jest.SpyInstance;
+
+beforeEach(() => {
+ warnSpy = jest.spyOn(logging, 'warn').mockImplementation(() => {});
+});
+
+afterEach(() => {
+ warnSpy.mockRestore();
+});
+
+test('addTranslation adds a key-value translation', () => {
+ const translator = new Translator();
+ translator.addTranslation('hello', ['Hello World']);
+ expect(translator.translate('hello')).toBe('Hello World');
+});
+
+test('addTranslations adds multiple translations from an object', () => {
+ const translator = new Translator();
+ translator.addTranslations({
+ 'key one': ['value one'],
+ 'key two': ['value two'],
+ });
+ expect(translator.translate('key one')).toBe('value one');
+ expect(translator.translate('key two')).toBe('value two');
+});
+
+test('addTranslations warns on null input', () => {
+ const translator = new Translator();
+ translator.addTranslations(null as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid translations');
+});
+
+test('addTranslations warns on array input', () => {
+ const translator = new Translator();
+ translator.addTranslations(['value'] as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid translations');
+});
+
+test('addLocaleData adds translations for the current locale', () => {
+ const translator = new Translator();
+ translator.addLocaleData({ en: { greeting: ['Hello!'] } });
+ expect(translator.translate('greeting')).toBe('Hello!');
+});
+
+test('addLocaleData falls back to English when current locale has no data', () => {
+ const translator = new Translator({
+ languagePack: {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n != 1)',
+ },
+ },
+ },
+ },
+ });
+ translator.addLocaleData({ en: { bonjour: ['Hello from en!'] } });
+ expect(translator.translate('bonjour')).toBe('Hello from en!');
+});
+
+test('addLocaleData warns when locale data has no matching locale', () => {
+ const translator = new Translator();
+ translator.addLocaleData({} as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid locale data');
+});
+
+test('translate returns the input key when Jed throws', () => {
+ const translator = new Translator();
+ (translator as any).i18n = {
+ translate: () => {
+ throw new Error('jed error');
+ },
+ };
+ expect(translator.translate('error key')).toBe('error key');
+});
+
+test('translateWithNumber returns a string when translating with number as first arg', () => {
+ const translator = new Translator();
+ translator.addTranslation('%(num)d item', ['%(num)d item', '%(num)d items']);
+ const result = translator.translateWithNumber(
+ '%(num)d item',
+ 1,
+ '%(num)d item',
+ '%(num)d items',
+ );
+ expect(typeof result).toBe('string');
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertions</b></div>
<div id="fix">
The test only checks that translateWithNumber returns a string but does not validate the actual translated content or interpolation. This could miss bugs in plural selection or formatting. Assert the expected output per BITO.md rule on asserting behavior logic.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
test('translateWithNumber returns a string when translating with number as first arg', () => {
const translator = new Translator();
translator.addTranslation('%(num)d item', ['%(num)d item', '%(num)d items']);
const result = translator.translateWithNumber(
'%(num)d item',
1,
'%(num)d item',
'%(num)d items',
);
expect(typeof result).toBe('string');
expect(result).toBe('1 item');
});
````
</div>
</details>
</div>
<small><i>Code Review Run #f50da4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-core/src/translation/Translator.test.ts:
##########
@@ -0,0 +1,132 @@
+/**
+ * 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 Translator from './Translator';
+import logging from '../utils/logging';
+
+let warnSpy: jest.SpyInstance;
+
+beforeEach(() => {
+ warnSpy = jest.spyOn(logging, 'warn').mockImplementation(() => {});
+});
+
+afterEach(() => {
+ warnSpy.mockRestore();
+});
+
+test('addTranslation adds a key-value translation', () => {
+ const translator = new Translator();
+ translator.addTranslation('hello', ['Hello World']);
+ expect(translator.translate('hello')).toBe('Hello World');
+});
+
+test('addTranslations adds multiple translations from an object', () => {
+ const translator = new Translator();
+ translator.addTranslations({
+ 'key one': ['value one'],
+ 'key two': ['value two'],
+ });
+ expect(translator.translate('key one')).toBe('value one');
+ expect(translator.translate('key two')).toBe('value two');
+});
+
+test('addTranslations warns on null input', () => {
+ const translator = new Translator();
+ translator.addTranslations(null as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid translations');
+});
+
+test('addTranslations warns on array input', () => {
+ const translator = new Translator();
+ translator.addTranslations(['value'] as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid translations');
+});
+
+test('addLocaleData adds translations for the current locale', () => {
+ const translator = new Translator();
+ translator.addLocaleData({ en: { greeting: ['Hello!'] } });
+ expect(translator.translate('greeting')).toBe('Hello!');
+});
+
+test('addLocaleData falls back to English when current locale has no data', () => {
+ const translator = new Translator({
+ languagePack: {
+ domain: 'superset',
+ locale_data: {
+ superset: {
+ '': {
+ domain: 'superset',
+ lang: 'fr',
+ plural_forms: 'nplurals=2; plural=(n != 1)',
+ },
+ },
+ },
+ },
+ });
+ translator.addLocaleData({ en: { bonjour: ['Hello from en!'] } });
+ expect(translator.translate('bonjour')).toBe('Hello from en!');
+});
+
+test('addLocaleData warns when locale data has no matching locale', () => {
+ const translator = new Translator();
+ translator.addLocaleData({} as any);
+ expect(warnSpy).toHaveBeenCalledWith('Invalid locale data');
+});
+
+test('translate returns the input key when Jed throws', () => {
+ const translator = new Translator();
+ (translator as any).i18n = {
+ translate: () => {
+ throw new Error('jed error');
+ },
+ };
+ expect(translator.translate('error key')).toBe('error key');
+});
+
+test('translateWithNumber returns a string when translating with number as first arg', () => {
+ const translator = new Translator();
+ translator.addTranslation('%(num)d item', ['%(num)d item', '%(num)d items']);
+ const result = translator.translateWithNumber(
+ '%(num)d item',
+ 1,
+ '%(num)d item',
+ '%(num)d items',
+ );
+ expect(typeof result).toBe('string');
+});
+
+test('translateWithNumber returns a string when translating with plural string as first arg', () => {
+ const translator = new Translator();
+ translator.addTranslation('%(num)d item', ['%(num)d item', '%(num)d items']);
+ const result = translator.translateWithNumber(
+ '%(num)d item',
+ '%(num)d item',
+ 2,
+ );
+ expect(typeof result).toBe('string');
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak test assertions</b></div>
<div id="fix">
Similar to the prior test, this only checks return type without validating plural translation output. Add assertion for the expected plural string to ensure correct behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
test('translateWithNumber returns a string when translating with plural string as first arg', () => {
const translator = new Translator();
translator.addTranslation('%(num)d item', ['%(num)d item', '%(num)d items']);
const result = translator.translateWithNumber(
'%(num)d item',
'%(num)d item',
2,
);
expect(typeof result).toBe('string');
expect(result).toBe('2 items');
});
````
</div>
</details>
</div>
<small><i>Code Review Run #f50da4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tests): achieve 100% TypeScript coverage for core packages [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38518:
URL: https://github.com/apache/superset/pull/38518#issuecomment-4024767133
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tests): achieve 100% TypeScript coverage for core packages [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38518:
URL: https://github.com/apache/superset/pull/38518#issuecomment-4024045524
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org