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: [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]
