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

Improved SearchForm test suite #688

Merged
merged 5 commits into from
Feb 19, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ export function mapStateToProps(state) {
};
}

function mapDispatchToProps(dispatch) {
export function mapDispatchToProps(dispatch) {
const { searchTraces } = bindActionCreators(jaegerApiActions, dispatch);
return {
onSubmit: fields => submitForm(fields, searchTraces),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
convTagsLogfmt,
getUnixTimeStampInMSFromForm,
lookbackToTimestamp,
mapDispatchToProps,
mapStateToProps,
optionsWithinMaxLookback,
submitForm,
Expand Down Expand Up @@ -264,6 +265,23 @@ describe('submitForm()', () => {
expect(operation).toBe(undefined);
});

it('expects operation to be value defined in beforeEach', () => {
submitForm(fields, searchTraces);
const { calls } = searchTraces.mock;
expect(calls.length).toBe(1);
const { operation } = calls[0][0];
expect(operation).toBe('op-a');
});

it('expects operation to be value assigned before call is made', () => {
fields.operation = 'test';
submitForm(fields, searchTraces);
const { calls } = searchTraces.mock;
expect(calls.length).toBe(1);
const { operation } = calls[0][0];
expect(operation).toBe('test');
});

describe('`fields.lookback`', () => {
function getCalledDuration(mock) {
const { start, end } = mock.calls[0][0];
Expand Down Expand Up @@ -414,6 +432,24 @@ describe('<SearchForm>', () => {
const field = wrapper.find(`Field[name="resultsLimit"]`);
expect(field.prop('props').max).toEqual(maxLimit);
});

it('disables operation when no service selected', () => {
wrapper = shallow(<SearchForm {...defaultProps} selectedService="" />);
const field = wrapper.find(`Field[name="operation"]`);
expect(field).toMatchSnapshot();
});

it('enables operation when unknown service selected', () => {
wrapper = shallow(<SearchForm {...defaultProps} selectedService="svcC" />);
const field = wrapper.find(`Field[name="operation"]`);
expect(field).toMatchSnapshot();
});

it('enables operation when known service selected', () => {
wrapper = shallow(<SearchForm {...defaultProps} selectedService="svcB" />);
const field = wrapper.find(`Field[name="operation"]`);
expect(field).toMatchSnapshot();
});
Copy link
Member

@yurishkuro yurishkuro Feb 17, 2021

Choose a reason for hiding this comment

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

If I understand this correctly, these new tests are merely checking a single property on the field. Wouldn't it be simpler to test that single property directly, like expect(field.foo).equalTo('bar'), than to compare with a whole snapshot? The intent would be a lot more obvious and maintenance easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I reworked those tests to test the single property instead of using the snapshots. Please let me know what you think, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

What about this specific test? Is it aiming to check only the boolean condition, or also the list of operations that are included in the dropdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, meant to remove that snapshot test in the last commit, I'm new to open source contributions so my bad!

});

describe('validation', () => {
Expand All @@ -439,7 +475,7 @@ describe('mapStateToProps()', () => {
let state;

beforeEach(() => {
state = { router: { location: { serach: '' } } };
state = { router: { location: { search: '' } } };
});

it('does not explode when the query string is empty', () => {
Expand Down Expand Up @@ -542,3 +578,11 @@ describe('mapStateToProps()', () => {
expect(msDiff(dateParams.dateStr, dateParams.dateTimeStr, endDate, endDateTime)).toBeLessThan(60 * 1000);
});
});

describe('mapDispatchToProps()', () => {
it('creates the actions correctly', () => {
expect(mapDispatchToProps(() => {})).toEqual({
onSubmit: expect.any(Function),
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`<SearchForm> disables operation when no service selected 1`] = `
<Field
component={[Function]}
name="operation"
placeholder="Select An Operation"
props={
Object {
"clearable": false,
"disabled": true,
"options": Array [
Object {
"label": "all",
"title": "all",
"value": "all",
},
],
"required": true,
}
}
/>
`;

exports[`<SearchForm> enables operation when known service selected 1`] = `
<Field
component={[Function]}
name="operation"
placeholder="Select An Operation"
props={
Object {
"clearable": false,
"disabled": false,
"options": Array [
Object {
"label": "all",
"title": "all",
"value": "all",
},
Object {
"label": "A",
"title": "A",
"value": "A",
},
Object {
"label": "B",
"title": "B",
"value": "B",
},
],
"required": true,
}
}
/>
`;

exports[`<SearchForm> enables operation when unknown service selected 1`] = `
<Field
component={[Function]}
name="operation"
placeholder="Select An Operation"
props={
Object {
"clearable": false,
"disabled": false,
"options": Array [
Object {
"label": "all",
"title": "all",
"value": "all",
},
],
"required": true,
}
}
/>
`;