Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(monorepo): change coverage of core to 100% #17698

Merged
merged 12 commits into from
Dec 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .codecov.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ coverage:
target: auto
threshold: 0%
core-packages-ts:
target: 95%
target: 100%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

paths:
- 'superset-frontend/packages'
- '!superset-frontend/packages/**/*.jsx'
Expand Down
7 changes: 4 additions & 3 deletions superset-frontend/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ module.exports = {
coverageReporters: ['lcov', 'json-summary', 'html'],
transform: {
'^.+\\.jsx?$': 'babel-jest',
// ts-jest can't load plugin 'babel-plugin-typescript-to-proptypes'
'reactify\\.tsx$': 'babel-jest',
'^.+\\.tsx?$': 'ts-jest',
// ts-jest doesn't work with `--coverage`. @superset-ui/core should
// 100% coverage, so we use babel-jest in packages and plugins.
'(plugins|packages)\\/.+\\.tsx?$': 'babel-jest',
'(((?!(plugins|packages)).)*)\\/.+\\.tsx?$': 'ts-jest',
},
moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json'],
snapshotSerializers: ['@emotion/jest/enzyme-serializer'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,7 @@ export default class CategoricalColorNamespace {
getScale(schemeId?: string) {
const id = schemeId ?? getCategoricalSchemeRegistry().getDefaultKey() ?? '';
const scheme = getCategoricalSchemeRegistry().get(id);
const newScale = new CategoricalColorScale(
scheme?.colors ?? [],
this.forcedItems,
);

return newScale;
return new CategoricalColorScale(scheme?.colors ?? [], this.forcedItems);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reformat

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,6 @@ import {
} from './types';
import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants';

function redirectUnauthorized() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function move into class

// the next param will be picked by flask to redirect the user after the login
setTimeout(() => {
window.location.href = `/login?next=${window.location.href}`;
});
}

export default class SupersetClientClass {
credentials: Credentials;

Expand Down Expand Up @@ -159,8 +152,8 @@ export default class SupersetClientClass {
timeout: timeout ?? this.timeout,
fetchRetryOptions: fetchRetryOptions ?? this.fetchRetryOptions,
}).catch(res => {
if (res && res.status === 401) {
redirectUnauthorized();
if (res?.status === 401) {
this.redirectUnauthorized();
}
return Promise.reject(res);
});
Expand Down Expand Up @@ -226,4 +219,8 @@ export default class SupersetClientClass {
endpoint[0] === '/' ? endpoint.slice(1) : endpoint
}`;
}

redirectUnauthorized() {
window.location.href = `/login?next=${window.location.href}`;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
import seedrandom from 'seedrandom';

let random = seedrandom('superset-ui');
import _seedrandom from 'seedrandom';

export function seed(seed: string) {
random = seedrandom(seed);
return random;
return _seedrandom(seed);
}

export function seedRandom() {
return random();
return _seedrandom('superset-ui')();
Comment on lines -19 to +26
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor refactor, to avoid duplicate names(seedrandom).

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
* under the License.
*/
import fetchMock from 'fetch-mock';
import {
SupersetClientClass,
ClientConfig,
} from '@superset-ui/core/src/connection';
import { SupersetClientClass, ClientConfig, CallApi } from '@superset-ui/core';
import { LOGIN_GLOB } from './fixtures/constants';

describe('SupersetClientClass', () => {
Expand Down Expand Up @@ -321,7 +318,7 @@ describe('SupersetClientClass', () => {
await client.init();
await client.get({ url: mockGetUrl });

const fetchRequest = fetchMock.calls(mockGetUrl)[0][1];
const fetchRequest = fetchMock.calls(mockGetUrl)[0][1] as CallApi;
expect(fetchRequest.mode).toBe(clientConfig.mode);
expect(fetchRequest.credentials).toBe(clientConfig.credentials);
expect(fetchRequest.headers).toEqual(
Expand Down Expand Up @@ -378,7 +375,7 @@ describe('SupersetClientClass', () => {
await client.init();
await client.get({ url: mockGetUrl, ...overrideConfig });

const fetchRequest = fetchMock.calls(mockGetUrl)[0][1];
const fetchRequest = fetchMock.calls(mockGetUrl)[0][1] as CallApi;
expect(fetchRequest.mode).toBe(overrideConfig.mode);
expect(fetchRequest.credentials).toBe(overrideConfig.credentials);
expect(fetchRequest.headers).toEqual(
Expand Down Expand Up @@ -423,7 +420,7 @@ describe('SupersetClientClass', () => {
await client.init();
await client.post({ url: mockPostUrl, ...overrideConfig });

const fetchRequest = fetchMock.calls(mockPostUrl)[0][1];
const fetchRequest = fetchMock.calls(mockPostUrl)[0][1] as CallApi;

expect(fetchRequest.mode).toBe(overrideConfig.mode);
expect(fetchRequest.credentials).toBe(overrideConfig.credentials);
Expand Down Expand Up @@ -454,7 +451,8 @@ describe('SupersetClientClass', () => {
await client.init();
await client.post({ url: mockPostUrl, postPayload });

const formData = fetchMock.calls(mockPostUrl)[0][1].body as FormData;
const fetchRequest = fetchMock.calls(mockPostUrl)[0][1] as CallApi;
const formData = fetchRequest.body as FormData;

expect(fetchMock.calls(mockPostUrl)).toHaveLength(1);
Object.entries(postPayload).forEach(([key, value]) => {
Expand All @@ -470,7 +468,8 @@ describe('SupersetClientClass', () => {
await client.init();
await client.post({ url: mockPostUrl, postPayload, stringify: false });

const formData = fetchMock.calls(mockPostUrl)[0][1].body as FormData;
const fetchRequest = fetchMock.calls(mockPostUrl)[0][1] as CallApi;
const formData = fetchRequest.body as FormData;

expect(fetchMock.calls(mockPostUrl)).toHaveLength(1);
Object.entries(postPayload).forEach(([key, value]) => {
Expand All @@ -479,4 +478,36 @@ describe('SupersetClientClass', () => {
});
});
});

it('should redirect Unauthorized', async () => {
const mockRequestUrl = 'https://host/get/url';
const { location } = window;
// @ts-ignore
delete window.location;
// @ts-ignore
window.location = { href: mockRequestUrl };
const authSpy = jest
.spyOn(SupersetClientClass.prototype, 'ensureAuth')
.mockImplementation();
const rejectValue = { status: 401 };
fetchMock.get(mockRequestUrl, () => Promise.reject(rejectValue), {
overwriteRoutes: true,
});

const client = new SupersetClientClass({});

let error;
try {
await client.request({ url: mockRequestUrl, method: 'GET' });
} catch (err) {
error = err;
} finally {
const redirectURL = window.location.href;
expect(redirectURL).toBe(`/login?next=${mockRequestUrl}`);
expect(error.status).toBe(401);
}

authSpy.mockReset();
window.location = location;
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@
* under the License.
*/

import { logging } from '@superset-ui/core';
import Translator from '@superset-ui/core/src/translation/Translator';
import {
logging,
configure,
t,
tn,
addLocaleData,
addTranslation,
addTranslations,
} from '@superset-ui/core/src/translation/TranslatorSingleton';
} from '@superset-ui/core';
import Translator from '../../src/translation/Translator';
import languagePackZh from './languagePacks/zh';
import languagePackEn from './languagePacks/en';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,8 @@

/* eslint no-console: 0 */
import mockConsole from 'jest-mock-console';
import Translator from '@superset-ui/core/src/translation/Translator';
import {
configure,
resetTranslation,
t,
tn,
} from '@superset-ui/core/src/translation/TranslatorSingleton';
import { configure, resetTranslation, t, tn } from '@superset-ui/core';
import Translator from '../../src/translation/Translator';

import languagePackEn from './languagePacks/en';
import languagePackZh from './languagePacks/zh';
Expand Down Expand Up @@ -76,4 +71,16 @@ describe('TranslatorSingleton', () => {
});
});
});
it('should be reset translation setting', () => {
configure();
expect(t('second')).toEqual('second');

resetTranslation();
const restoreConsole = mockConsole();
expect(t('second')).toEqual('second');
resetTranslation();
expect(t('second')).toEqual('second');
expect(console.warn).toBeCalledTimes(2);
restoreConsole();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { configure, t, tn } from '@superset-ui/core/src/translation';
import { configure, t, tn } from '@superset-ui/core';

describe('index', () => {
it('exports configure()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { LanguagePack } from '@superset-ui/core/src/translation';
import { LanguagePack } from '@superset-ui/core';

const languagePack: LanguagePack = {
domain: 'superset',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { LanguagePack } from '@superset-ui/core/src/translation';
import { LanguagePack } from '@superset-ui/core';

const languagePack: LanguagePack = {
domain: 'superset',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,40 +21,41 @@
describe('logging', () => {
beforeEach(() => {
jest.resetModules();
// Explicit is better than implicit
console.warn = console.error = function mockedConsole(message) {
throw new Error(message);
};
jest.resetAllMocks();
});

it('should pipe to `console` methods', () => {
const { logging } = require('@superset-ui/core/src');
const { logging } = require('@superset-ui/core');

jest.spyOn(logging, 'debug').mockImplementation();
jest.spyOn(logging, 'log').mockImplementation();
jest.spyOn(logging, 'info').mockImplementation();
expect(() => {
logging.debug();
logging.log();
logging.info();
}).not.toThrow();
expect(() => {
logging.warn('warn');
}).toThrow('warn');
expect(() => {
logging.error('error');
}).toThrow('error');
Comment on lines +30 to -42
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock console output, to make clear test output.

before

(superset) yongjie.zhao@:superset-frontend$ rm -rf coverage && npx jest --clearCache && NODE_ENV=test npx jest packages/superset-ui-core/test/utils/logging.test.ts
Cleared /private/var/folders/10/0l8mvpf52jx6t3p68mndwn7m0000gn/T/jest_dx
 PASS  packages/superset-ui-core/test/utils/logging.test.ts (9.898 s)
  logging
    ✓ should pipe to `console` methods (4487 ms)
    ✓ should use noop functions when console unavailable (72 ms)

  console.debug
    undefined

      at packages/superset-ui-core/test/utils/logging.test.ts:33:15

  console.log
    undefined

      at packages/superset-ui-core/test/utils/logging.test.ts:34:15

  console.info
    undefined

      at packages/superset-ui-core/test/utils/logging.test.ts:35:15

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        10.875 s
Ran all test suites matching /packages\/superset-ui-core\/test\/utils\/logging.test.ts/i.

After

(superset) yongjie.zhao@:superset-frontend$ rm -rf coverage && npx jest --clearCache && NODE_ENV=test npx jest packages/superset-ui-core/test/utils/logging.test.ts
Cleared /private/var/folders/10/0l8mvpf52jx6t3p68mndwn7m0000gn/T/jest_dx
 PASS  packages/superset-ui-core/test/utils/logging.test.ts (8.997 s)
  logging
    ✓ should pipe to `console` methods (4409 ms)
    ✓ should use noop functions when console unavailable (84 ms)

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        9.707 s
Ran all test suites matching /packages\/superset-ui-core\/test\/utils\/logging.test.ts/i.


// to support: npx jest --silent
const spy = jest.spyOn(logging, 'trace');
spy.mockImplementation(() => {
jest.spyOn(logging, 'warn').mockImplementation(() => {
throw new Error('warn');
});
expect(() => logging.warn()).toThrow('warn');

jest.spyOn(logging, 'error').mockImplementation(() => {
throw new Error('error');
});
expect(() => logging.error()).toThrow('error');

jest.spyOn(logging, 'trace').mockImplementation(() => {
throw new Error('Trace:');
});
expect(() => {
logging.trace();
}).toThrow('Trace:');
spy.mockRestore();
expect(() => logging.trace()).toThrow('Trace:');
});

it('should use noop functions when console unavailable', () => {
const { console } = window;
Object.assign(window, { console: undefined });
const { logging } = require('@superset-ui/core/src');
const { logging } = require('@superset-ui/core');

afterAll(() => {
Object.assign(window, { console });
Expand Down