Don't rely on json-markup for non-json strings #587

smanolloff · 2020-05-25T19:01:17Z

Which problem is this PR solving?

Resolves #571

The presence of those quotes around strings is not only unnecessary (the value type is inferred from the text color), but also confusing (when the value itself contains quotes)

Short description of the changes

Added a separate function in case the value is a non-json string.

Before:

After:

yurishkuro · 2020-05-25T20:10:42Z

Thanks for the PR. Please make sure the CI is green and all commits are signed.

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

codecov · 2020-05-26T00:28:12Z

Codecov Report

Merging #587 into master will decrease coverage by 0.02%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   93.55%   93.52%   -0.03%     
==========================================
  Files         217      217              
  Lines        5289     5296       +7     
  Branches     1360     1362       +2     
==========================================
+ Hits         4948     4953       +5     
- Misses        300      302       +2     
  Partials       41       41

Impacted Files	Coverage Δ
.../TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx	`96.55% <92.30%> (-3.45%)`	⬇️
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	`88.13% <0.00%> (-1.70%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3335512...52e64dd. Read the comment docs.

yurishkuro · 2020-05-26T01:09:22Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.tsx

  // if the value is a string representing actual json object or array, then use json-markup
-  if (typeof value === 'string' && jsonObjectOrArrayStartRegex.test(value)) {
+  if (typeof value === 'string') {


Why does this logic need to change if the goal is to only remove the quotes? Attempting to parse any value as JSON led to bugs previously, where, for example, a string that looks like a number would get converted to number and change representation / precision.

but it has not changed - see a few lines below.
Basically, the boolean && has been split into two separate conditionals, due to the need to explicitly handle non-json strings.

ok, thanks. Could you please add unit tests that cover the new conditions? There are only two lines that are not covered: https://codecov.io/gh/jaegertracing/jaeger-ui/compare/3335512110c60ace5b089e1b61d187a6d6ec4000...6c76e930f59843f41817effbf3d57c3f3a999367/diff

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

yurishkuro · 2020-05-27T20:27:16Z

...ges/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.test.js

@@ -47,8 +47,8 @@ describe('<KeyValuesTable>', () => {

  const data = [
    { key: 'span.kind', value: 'client' },
-    { key: 'omg', value: 'mos-def' },
-    { key: 'numericString', value: '12345678901234567890' },


please keep this, it's a real use case that cased problems in the past.

yurishkuro · 2020-05-27T20:28:43Z

...ges/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.test.js

@@ -138,7 +138,7 @@ describe('<KeyValuesTable>', () => {
    expect(el.length).toBe(data.length);
    el.forEach((valueDiv, i) => {
      if (data[i].key !== 'jsonkey') {
-        expect(valueDiv.html()).toMatch(`"${data[i].value}"`);
+        expect(valueDiv.html()).toMatch(data[i].value.toString());


Could we change the data table to contain the exact expectation instead of relying on toString()?

I assume you are well aware that the string interpolation was basically doing the same thing.
I did try to make this test more meaningful by comparing against the actual text instead of matching against part of the DOM, but it did not work:

// ----------- this works valueDiv.html() // => "<div class="ub-inline-block"><div class="json-markup"><span class="json-markup-string">some text</span></div></div>" valueDiv.text() // => "some text" // ----------- this not valueDiv.html() // => "<div class="ub-inline-block"><div><div class="json-markup"><span class="json-markup-number">some text</span></div></div></div>" valueDiv.text() // => ""

I know it is somehow related to the shallow representation of the DOM inside this test, but I am really not familiar with react and its test frameworks, so I am at a dead end.
If you can help me get "some text" in the second example, I would implement the test change.

sorry, maybe I didn't explain clearly. The test is data driven from the data table. I suggest adding the expected values to that table, e.g. { key: 'numeric', value: 123456789, expected: "123456789" } and comparing with those, instead of value.toString() (which is not as obvious in terms of what it would produce).

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

yurishkuro

thanks! 👏

yurishkuro · 2020-05-29T20:06:07Z

...ges/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.test.js

+    { key: 'span.kind', value: 'client', expected: 'client' },
+    { key: 'omg', value: 'mos-def', expected: 'mos-def' },
+    { key: 'numericString', value: '12345678901234567890', expected: '12345678901234567890' },
+    { key: 'numeric', value: 123456789, expected: '123456789' },
    { key: 'jsonkey', value: JSON.stringify({ hello: 'world' }) },


sorry, how does this one pass if it doesn't have expected field?

jaeger-ui/packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/KeyValuesTable.test.js

Line 141 in 52e64dd

if (data[i].key !== 'jsonkey') {

* don't rely on json-markup for non-json strings Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> * prettier Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> * fix quotes around non-json strings; test coverage Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> * lint Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> * prettier Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> * don't use toString in tests Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

smanolloff requested review from everett980 and tiffon as code owners May 25, 2020 19:01

smanolloff force-pushed the no-quotes-around-strings branch from 8ba12a1 to 1f54d3a Compare May 25, 2020 19:07

don't rely on json-markup for non-json strings

20fb5ec

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

smanolloff force-pushed the no-quotes-around-strings branch from 1f54d3a to abd1383 Compare May 26, 2020 00:18

prettier

6c76e93

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

smanolloff force-pushed the no-quotes-around-strings branch from abd1383 to 6c76e93 Compare May 26, 2020 00:19

yurishkuro reviewed May 26, 2020

View reviewed changes

fix quotes around non-json strings; test coverage

de51eb6

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

smanolloff force-pushed the no-quotes-around-strings branch from 801a528 to de51eb6 Compare May 27, 2020 14:11

lint

70cb812

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

smanolloff force-pushed the no-quotes-around-strings branch from 1dcd55a to 70cb812 Compare May 27, 2020 14:20

prettier

b1671da

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

yurishkuro reviewed May 27, 2020

View reviewed changes

don't use toString in tests

52e64dd

Signed-off-by: Simeon Manolov <simeon.manolov@sumup.com>

yurishkuro approved these changes May 29, 2020

View reviewed changes

yurishkuro reviewed May 29, 2020

View reviewed changes

yurishkuro merged commit 4dbd576 into jaegertracing:master Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't rely on json-markup for non-json strings #587

Don't rely on json-markup for non-json strings #587

smanolloff commented May 25, 2020 •

edited by yurishkuro

Loading

yurishkuro commented May 25, 2020

codecov bot commented May 26, 2020 •

edited

Loading

yurishkuro May 26, 2020

smanolloff May 26, 2020

yurishkuro May 27, 2020

yurishkuro May 27, 2020

yurishkuro May 27, 2020

smanolloff May 28, 2020 •

edited

Loading

yurishkuro May 28, 2020

yurishkuro left a comment

yurishkuro May 29, 2020

smanolloff Jun 1, 2020

Don't rely on json-markup for non-json strings #587

Don't rely on json-markup for non-json strings #587

Conversation

smanolloff commented May 25, 2020 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro commented May 25, 2020

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smanolloff May 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smanolloff commented May 25, 2020 •

edited by yurishkuro

Loading

codecov bot commented May 26, 2020 •

edited

Loading

smanolloff May 28, 2020 •

edited

Loading