Fix search.maxLimit configuration #533
Conversation
Signed-off-by: Ruben Vargas <[email protected]>
rubenvp8510
changed the title
| @@ -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'; | |||
There was a problem hiding this comment.
should there be a unit test that would catch this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll add tests. not sure why this is not in the docker image, may be an issue related too the release and dockerization.
There was a problem hiding this comment.
Docker image contains JS transpiled to ES5. I'd assume the code to look very different there (also minified).
Codecov Report
@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 92.95% 92.97% +0.02%
==========================================
Files 197 197
Lines 4840 4840
Branches 1174 1174
==========================================
+ Hits 4499 4500 +1
+ Misses 301 300 -1
Partials 40 40
Continue to review full report at Codecov.
|
|
Tests added. |
| @@ -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 = Math.floor(Math.random() * 7000) + 3000; | |||
There was a problem hiding this comment.
a fixed constant would do just as well, e.g. 1234 (unlikely to be the default value elsewhere)
There was a problem hiding this comment.
Ok, I'll use a constant.
| 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); |
There was a problem hiding this comment.
did you confirm that this test breaks without the fix above?
There was a problem hiding this comment.
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}}
Signed-off-by: Ruben Vargas <[email protected]>
rubenvp8510
force-pushed
the
fix-search-limit
branch
from
a130222
to
da5445b
Compare
|
@rubenvp8510 is this ready to be merged? @yurishkuro can this be merged so 1.17.1 can be released? |
|
@pavolloffay if there is no more comments, is ready. |
* Fix the way of getting search.maxLimit configuration Signed-off-by: Ruben Vargas <[email protected]> * Add tests for srarch.maxLimit configuration Signed-off-by: Ruben Vargas <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: vvvprabhakar <[email protected]>
Signed-off-by: Ruben Vargas [email protected]
Which problem is this PR solving?
Short description of the changes
getConfigValueis used instead of the default exportedgetConfig