Downgrade react-router-dom (#803) #837
Conversation
Signed-off-by: Waldemar Penner <[email protected]>
w0wka91
requested review from
everett980,
rubenvp8510 and
tiffon
as code owners
|
Thanks for the investigation and PR. We should really have some unit tests to catch a major function breaking silently like this. |
Signed-off-by: Waldemar Penner <[email protected]>
|
Not sure if unit tests will be sufficient to test this because every unit did work correct but the combination of react-router-dom and react-router-redux was broken. |
|
A discussion of the UI testing techniques is a bit outside of my area of expertise. cc @jkowall |
Codecov Report
@@ Coverage Diff @@
## master #837 +/- ##
==========================================
- Coverage 95.29% 95.26% -0.03%
==========================================
Files 240 240
Lines 7460 7460
Branches 1812 1812
==========================================
- Hits 7109 7107 -2
- Misses 345 347 +2
Partials 6 6
Continue to review full report at Codecov.
|
|
There was indeed a conversation about introducing Cypress, and even a presentation delivered by an Outreachy candidate a few months ago on the topic. IMO, it would be great to have it. |
|
I think the integration of cypress should be not done in the scope of this PR. So I would suggest we merge this PR, close this Issue and create a new Issue which is about the integration of cypress. What do you guys think? |
|
Sounds like a plan! @rubenvp8510, could you do a final review before I merge this? |
|
cc @meenal06 who made the original change |
|
We could also just revert the PR. The original PR just changed some formatting and made 2 simple changes to tests. So either way should be fine |
|
@w0wka91 the change to packages/jaeger-ui/src/index.js looks significant, but not present in your PR. I prefer not to roll back #727 completely if it had useful forward-looking changes. |
|
Should this be considered for tomorrow's release? |
|
Yes. We need someone w/ React knowledge to look at this |
|
@rubenvp8510, could you do a final review? |
Sure! I did a quick review and looks good. I'll do a final review and approve if everything is OK. |
## Which problem is this PR solving? - part of: #1825 - Upgrades react-router-dom to v5.2.0 ## Description of the changes - This PR upgrades the rrd to v5.2.0 - This upgrade was previously attempted with PR #727 but at that time, an issue (#803) was reported because `react-router-redux` v5.x was not compatible with rrd v5.2.0, so it was reverted with PR: #837 - Now, since we have `redux-first-history` instead of `react-router-redux`, we can upgrade to rrd v5.2.0 safely now. ## How was this change tested? - The reported issue with v5.2.0 (#803) is not being reproduced now.  ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
Signed-off-by: Waldemar Penner [email protected]
Which problem is this PR solving?
Short description of the changes