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

Fix search.maxLimit configuration #533

Merged
merged 3 commits into from
Mar 9, 2020
Merged

Fix search.maxLimit configuration #533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dad9a1
Fix the way of getting search.maxLimit configuration
rubenvp8510 Feb 26, 2020
da5445b
Add tests for srarch.maxLimit configuration
rubenvp8510 Feb 27, 2020
771c4d0
Merge branch 'master' into fix-search-limit
yurishkuro Mar 9, 2020
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 packages/jaeger-ui/src/components/SearchTracePage/SearchForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import * as jaegerApiActions from '../../actions/jaeger-api';
import { formatDate, formatTime } from '../../utils/date';
import reduxFormFieldAdapter from '../../utils/redux-form-field-adapter';
import { DEFAULT_OPERATION, DEFAULT_LIMIT, DEFAULT_LOOKBACK } from '../../constants/search-form';
import getConfigValue from '../../utils/config/get-config';
import { getConfigValue } from '../../utils/config/get-config';
Copy link
Member

Choose a reason for hiding this comment

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

should there be a unit test that would catch this?

Copy link

Choose a reason for hiding this comment

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

Also, here maybe problem with build, package, dockerize etc. Because I did not see this deleted 37th row, when I am debugging in Chrome. I suggest testing after building into docker.

Copy link
Collaborator Author

@rubenvp8510 rubenvp8510 Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'll add tests. not sure why this is not in the docker image, may be an issue related too the release and dockerization.

Copy link
Member

Choose a reason for hiding this comment

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

Docker image contains JS transpiled to ES5. I'd assume the code to look very different there (also minified).

Copy link

Choose a reason for hiding this comment

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

image
screenshot from chrome devtools.

import './SearchForm.css';

const FormItem = Form.Item;
Expand Down
13 changes: 13 additions & 0 deletions packages/jaeger-ui/src/components/SearchTracePage/SearchForm.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,19 @@ describe('<SearchForm>', () => {
btn = wrapper.find(`[data-test="${markers.SUBMIT_BTN}"]`);
expect(btn.prop('disabled')).toBeTruthy();
});

it('uses config.search.maxLimit', () => {
const maxLimit = 6789;
const config = {
search: {
maxLimit,
},
};
window.getJaegerUiConfig = jest.fn(() => config);
wrapper = shallow(<SearchForm {...defaultProps} selectedService="svcA" />);
const field = wrapper.find(`Field[name="resultsLimit"]`);
expect(field.prop('props').max).toEqual(maxLimit);
Copy link
Member

Choose a reason for hiding this comment

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

did you confirm that this test breaks without the fix above?

Copy link
Collaborator Author

@rubenvp8510 rubenvp8510 Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes

Without this fix:

Error: expect(received).toEqual(expected)

Expected value to equal:
  4854
Received:
 {"archiveEnabled": false, "dependencies": {"dagMaxNumServices": 100, "menuEnabled": true}, "linkPatterns": [], "menu": [{"items": [{"label": "GitHub", "url": "https://github.com/uber/jaeger"}, {"label": "Docs", "url": "http://jaeger.readthedocs.io/en/latest/"}, {"label": "Twitter", "url": "https://twitter.com/JaegerTracing"}, {"label": "Discussion Group", "url": "https://groups.google.com/forum/#!forum/jaeger-tracing"}, {"label": "Gitter.im", "url": "https://gitter.im/jaegertracing/Lobby"}, {"label": "Blog", "url": "https://medium.com/jaegertracing/"}], "label": "About Jaeger"}], "search": {"maxLimit": 4854, "maxLookback": {"label": "2 Days", "value": "2d"}}, "tracking": {"gaID": null, "trackErrors": true}}

});
});

describe('validation', () => {
Expand Down