feat: upgrade to react-router-dom v5.2.0

[meenal06]

Signed-off-by: Meenal Trivedi meenaltrivedi6102@gmail.com

Which problem is this PR solving?

• Resolves Check what's needed to update react-router-dom from 4.3.1 to 5.2.0 #725

Short description of the changes

• This PR updates react-router-dom to v5.2.0 and fixes the issues due to the upgrade.

[meenal06]

Hi @rubenvp8510 , I tested and built jaeger-ui on local and it was working flawless. However, since I have modified the main application code but one unit-test is failing due to the old snapshot. Can you please tell me how can I update the snapshot stored for the ?

[yurishkuro]

@meenal06 small nit: say Resolves #725 instead of just #725, because the former will cause the ticket to auto-close once the PR is merged. I am going to update the description now.

[meenal06]

@meenal06 small nit: say Resolves #725 instead of just #725, because the former will cause the ticket to auto-close once the PR is merged. I am going to update the description now.

Thank you! I will be sure to keep this in mind for my future PRs 😄

[rubenvp8510]

Hi @meenal06 to update the test snapshots you can run run yarn test --u under the directory packages/jaeger-ui. This command will update all the necessary snapshots. (note the -u flag).

Once you fixes the snapshot errors, there are a couple of more tests that are failing. I'll try to check tomorrow If I got a chance, but if you fixes it and update the PR ask me for a review again, I'll glad to do it.

[meenal06]

Hi @rubenvp8510 , I updated the snapshot but alas there are other failing tests most probably due to the router issues which I am not able to fix as I am completely new to jest framework. Do let me know if you may find the cause of issue.

[rubenvp8510]

Hi @rubenvp8510 , I updated the snapshot but alas there are other failing tests most probably due to the router issues which I am not able to fix as I am completely new to jest framework. Do let me know if you may find the cause of issue.

Hi @meenal06 what is happening is that you cannot mutate the matchPath method of the module 'react-router-dom', because it only has a getter, potential solution would be to mock the entire 'react-router' which is the module from ''react-router-dom' re-export the matchPath functions.

Look at this , could help you to understand better what is happening:

babel/babel#8363
https://stackoverflow.com/questions/53162001/typeerror-during-jests-spyon-cannot-set-property-getrequest-of-object-which

[meenal06]

Hi @rubenvp8510 , thank you very much for the suggestion. I have fixed all the tests and upgraded to react-router-dom v5.2, please review!

[codecov]

Codecov Report

Merging #727 (5efecb2) into master (ae758fa) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 5efecb2 differs from pull request most recent head b9a30dd. Consider uploading reports for the commit b9a30dd to get more accurate results

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
- Coverage   94.39%   94.37%   -0.02%     
==========================================
  Files         230      230              
  Lines        5959     5960       +1     
  Branches     1448     1448              
==========================================
  Hits         5625     5625              
- Misses        300      301       +1     
  Partials       34       34              

Impacted Files	Coverage Δ
packages/jaeger-ui/src/index.js	0.00% <0.00%> (ø)

---

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 ae758fa...b9a30dd. Read the comment docs.

[meenal06]

I am not sure if the coverage issue highlighted by codecov was being covered before the test too, as I couldn't see any unit test file for the src/index.js 😕

[meenal06]

Hi @rubenvp8510 @yurishkuro please review I have changed the selector as there were issues due to the react-router upgrade !

[rubenvp8510]

Hi @rubenvp8510 @yurishkuro please review I have changed the selector as there were issues due to the react-router upgrade !

Hi, sorry I'll do a review today afternoon :)

[rubenvp8510]

LGTM

[yurishkuro]

Excellent work, @meenal06

[jpkrohling]

Nice job!

[meenal06]

Thank you team 🙂 !