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.

Sign up for GitHub

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

Jump to bottom

refactor: remove immutable from 'config' state slice #4960

Merged
merged 8 commits into from
Mar 11, 2021
Merged

refactor: remove immutable from 'config' state slice #4960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7892db8
refactor: remove immutable from 'config' state slice
smashercosmo Feb 15, 2021
637819d
fix: add type to config's state
smashercosmo Feb 17, 2021
57b96f6
fix: remove non public types
erezrokah Feb 28, 2021
84366c6
refactor: don't use default state in tests
erezrokah Feb 28, 2021
c4fa7e8
refactor: remove todos
erezrokah Feb 28, 2021
652bebf
refactor: code cleanup
erezrokah Mar 10, 2021
3e3afde
chore: fix lint error
erezrokah Mar 10, 2021
c0e43bc
fix: maintain collection config order
erezrokah Mar 10, 2021
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
4 changes: 2 additions & 2 deletions packages/netlify-cms-backend-gitlab/src/__tests__/gitlab.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ describe('gitlab backend', () => {
},
{
backendName: 'gitlab',
config: fromJS(config),
config,
authStore,
},
);
Expand Down Expand Up @@ -401,7 +401,7 @@ describe('gitlab backend', () => {

const entry = await backend.getEntry(
{
config: fromJS({}),
config: {},
integrations: fromJS([]),
entryDraft: fromJS({}),
mediaLibrary: fromJS({}),
Expand Down
16 changes: 16 additions & 0 deletions packages/netlify-cms-core/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,14 @@ declare module 'netlify-cms-core' {
cms_label_prefix?: string;
squash_merges?: boolean;
proxy_url?: string;
commit_messages?: {
create?: string;
update?: string;
delete?: string;
uploadMedia?: string;
deleteMedia?: string;
openAuthoring?: string;
};
}

export interface CmsSlug {
Expand Down Expand Up @@ -382,6 +390,14 @@ declare module 'netlify-cms-core' {
media_library?: CmsMediaLibrary;
publish_mode?: CmsPublishMode;
load_config_file?: boolean;
integrations?: {
hooks: string[];
provider: string;
collections?: '*' | string[];
applicationID?: string;
apiKey?: string;
getSignedFormURL?: string;
}[];
slug?: CmsSlug;
i18n?: CmsI18nConfig;
local_backend?: boolean | CmsLocalBackend;
Expand Down
59 changes: 23 additions & 36 deletions packages/netlify-cms-core/src/__tests__/backend.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import { Map, List, fromJS } from 'immutable';
import {
resolveBackend,
Backend,
extractSearchFields,
expandSearchEntries,
mergeExpandedEntries,
} from '../backend';
import registry from 'Lib/registry';
import { FOLDER } from 'Constants/collectionTypes';
import { Map, List, fromJS } from 'immutable';
import { FILES } from '../constants/collectionTypes';
import registry from '../lib/registry';
import { FOLDER, FILES } from '../constants/collectionTypes';

jest.mock('Lib/registry');
jest.mock('../lib/registry');
jest.mock('netlify-cms-lib-util');
jest.mock('../lib/urlHelper');

Expand All @@ -22,13 +21,11 @@ describe('Backend', () => {
registry.getBackend.mockReturnValue({
init: jest.fn(),
});
backend = resolveBackend(
Map({
backend: Map({
name: 'git-gateway',
}),
}),
);
backend = resolveBackend({
backend: {
name: 'git-gateway',
},
});
});

it('filters string values', () => {
Expand Down Expand Up @@ -133,9 +130,8 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erezrokah why do you think it's better to not use real default state? When tests are converted to typescript we'll have to either put a lot of tsignore statements or "as SomeType" statements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I worried this will make the tests brittle. Personally I like to have in the tests as little as information possible required for them to pass so I don't have to change them too often.

As for typing the tests - we'll have to see if that makes our lives easier or not? I would expect tests to fail when something breaks and not rely on types?

Copy link
Contributor Author

@smashercosmo smashercosmo Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would expect tests to fail when something breaks and not rely on types

this is true. On the other hand you're basically passing incorrect values to functions being tested. So tests might pass because of that. And might fail if correct values are provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is true. On the other hand you're basically passing incorrect values to functions being tested. So tests might pass because of that. And might fail if correct values are provided.

I believe that's something to verify when writing the tests. The test should be scoped to assert a specific thing and not verify that backend.ts methods work with the default config?

If so, we should have separate unit tests to verify that backend.ts handles the configuration properly (and not assume it works because we're passing default values).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright


const collection = Map({
name: 'posts',
Expand All @@ -155,9 +151,7 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

const collection = Map({
name: 'posts',
Expand All @@ -177,9 +171,8 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

const collection = Map({
name: 'posts',
Expand Down Expand Up @@ -218,9 +211,8 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

const collection = Map({
name: 'posts',
Expand Down Expand Up @@ -268,9 +260,8 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

backend.entryToRaw = jest.fn().mockReturnValue('');

Expand All @@ -295,9 +286,8 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

backend.entryToRaw = jest.fn().mockReturnValue('content');

Expand Down Expand Up @@ -334,10 +324,10 @@ describe('Backend', () => {
init: jest.fn(() => implementation),
persistMedia: jest.fn().mockResolvedValue(persistMediaResult),
};
const config = Map({});
const config = { backend: { name: 'github' } };

const backend = new Backend(implementation, { config, backendName: config.backend.name });
const user = { login: 'login', name: 'name' };
const backend = new Backend(implementation, { config, backendName: 'github' });
backend.currentUser = jest.fn().mockResolvedValue(user);

const file = { path: 'static/media/image.png' };
Expand Down Expand Up @@ -365,7 +355,9 @@ describe('Backend', () => {
.mockResolvedValueOnce('---\ntitle: "Hello World"\n---\n'),
unpublishedEntryMediaFile: jest.fn().mockResolvedValueOnce({ id: '1' }),
};
const config = Map({ media_folder: 'static/images' });
const config = {
media_folder: 'static/images',
};

const backend = new Backend(implementation, { config, backendName: 'github' });

Expand Down Expand Up @@ -412,8 +404,6 @@ describe('Backend', () => {
const { sanitizeSlug } = require('../lib/urlHelper');
sanitizeSlug.mockReturnValue('some-post-title');

const config = Map({});

const implementation = {
init: jest.fn(() => implementation),
getEntry: jest.fn(() => Promise.resolve()),
Expand All @@ -436,7 +426,7 @@ describe('Backend', () => {
title: 'some post title',
});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

await expect(backend.generateUniqueSlug(collection, entry, Map({}), [])).resolves.toBe(
'sub_dir/some-post-title',
Expand All @@ -448,8 +438,6 @@ describe('Backend', () => {
sanitizeSlug.mockReturnValue('some-post-title');
sanitizeChar.mockReturnValue('-');

const config = Map({});

const implementation = {
init: jest.fn(() => implementation),
getEntry: jest.fn(),
Expand All @@ -475,7 +463,7 @@ describe('Backend', () => {
title: 'some post title',
});

const backend = new Backend(implementation, { config, backendName: 'github' });
const backend = new Backend(implementation, { config: {}, backendName: 'github' });

await expect(backend.generateUniqueSlug(collection, entry, Map({}), [])).resolves.toBe(
'sub_dir/some-post-title-1',
Expand Down Expand Up @@ -585,11 +573,10 @@ describe('Backend', () => {
const implementation = {
init: jest.fn(() => implementation),
};
const config = Map({});

let backend;
beforeEach(() => {
backend = new Backend(implementation, { config, backendName: 'github' });
backend = new Backend(implementation, { config: {}, backendName: 'github' });
backend.listAllEntries = jest.fn(collection => {
if (collection.get('name') === 'posts') {
return Promise.resolve(posts);
Expand Down
9 changes: 4 additions & 5 deletions packages/netlify-cms-core/src/actions/__tests__/config.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { stripIndent } from 'common-tags';
import { fromJS } from 'immutable';
import {
loadConfig,
parseConfig,
Expand Down Expand Up @@ -932,13 +931,13 @@ describe('config', () => {
expect(dispatch).toHaveBeenCalledWith({ type: 'CONFIG_REQUEST' });
expect(dispatch).toHaveBeenCalledWith({
type: 'CONFIG_SUCCESS',
payload: fromJS({
payload: {
backend: { repo: 'test-repo' },
collections: [],
publish_mode: 'simple',
slug: { encoding: 'unicode', clean_accents: false, sanitize_replacement: '-' },
public_folder: '/',
}),
},
});
});

Expand All @@ -965,13 +964,13 @@ describe('config', () => {
expect(dispatch).toHaveBeenCalledWith({ type: 'CONFIG_REQUEST' });
expect(dispatch).toHaveBeenCalledWith({
type: 'CONFIG_SUCCESS',
payload: fromJS({
payload: {
backend: { repo: 'github' },
collections: [],
publish_mode: 'simple',
slug: { encoding: 'unicode', clean_accents: false, sanitize_replacement: '-' },
public_folder: '/',
}),
},
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/netlify-cms-core/src/actions/__tests__/entries.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,13 +395,13 @@ describe('entries', () => {

describe('validateMetaField', () => {
const state = {
config: fromJS({
config: {
slug: {
encoding: 'unicode',
clean_accents: false,
sanitize_replacement: '-',
},
}),
},
entries: fromJS([]),
};
const collection = fromJS({
Expand Down
42 changes: 21 additions & 21 deletions packages/netlify-cms-core/src/actions/__tests__/mediaLibrary.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import thunk from 'redux-thunk';
import { List, Map } from 'immutable';
import { insertMedia, persistMedia, deleteMedia } from '../mediaLibrary';

jest.mock('coreSrc/backend');
jest.mock('../../backend');
jest.mock('../waitUntil');
jest.mock('netlify-cms-lib-util', () => {
const lib = jest.requireActual('netlify-cms-lib-util');
Expand All @@ -20,9 +20,9 @@ describe('mediaLibrary', () => {
describe('insertMedia', () => {
it('should return mediaPath as string when string is given', () => {
const store = mockStore({
config: Map({
config: {
public_folder: '/media',
}),
},
collections: Map({
posts: Map({ name: 'posts' }),
}),
Expand All @@ -40,9 +40,9 @@ describe('mediaLibrary', () => {

it('should return mediaPath as array of strings when array of strings is given', () => {
const store = mockStore({
config: Map({
config: {
public_folder: '/media',
}),
},
collections: Map({
posts: Map({ name: 'posts' }),
}),
Expand Down Expand Up @@ -81,14 +81,14 @@ describe('mediaLibrary', () => {
getBlobSHA.mockReturnValue('000000000000000');

const store = mockStore({
config: Map({
config: {
media_folder: 'static/media',
slug: Map({
slug: {
encoding: 'unicode',
clean_accents: false,
sanitize_replacement: '-',
}),
}),
},
},
collections: Map({
posts: Map({ name: 'posts' }),
}),
Expand Down Expand Up @@ -132,14 +132,14 @@ describe('mediaLibrary', () => {

it('should persist media when not editing draft', () => {
const store = mockStore({
config: Map({
config: {
media_folder: 'static/media',
slug: Map({
slug: {
encoding: 'unicode',
clean_accents: false,
sanitize_replacement: '-',
}),
}),
},
},
collections: Map({
posts: Map({ name: 'posts' }),
}),
Expand Down Expand Up @@ -186,14 +186,14 @@ describe('mediaLibrary', () => {

it('should sanitize media name if needed when persisting', () => {
const store = mockStore({
config: Map({
config: {
media_folder: 'static/media',
slug: Map({
slug: {
encoding: 'ascii',
clean_accents: true,
sanitize_replacement: '_',
}),
}),
},
},
collections: Map({
posts: Map({ name: 'posts' }),
}),
Expand Down Expand Up @@ -247,9 +247,9 @@ describe('mediaLibrary', () => {

it('should delete non draft file', () => {
const store = mockStore({
config: Map({
config: {
publish_mode: 'editorial_workflow',
}),
},
collections: Map(),
integrations: Map(),
mediaLibrary: Map({
Expand Down Expand Up @@ -290,9 +290,9 @@ describe('mediaLibrary', () => {

it('should not delete a draft file', () => {
const store = mockStore({
config: Map({
config: {
publish_mode: 'editorial_workflow',
}),
},
collections: Map(),
integrations: Map(),
mediaLibrary: Map({
Expand Down
Loading