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.

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(discovery-client): fix off by one error in remove logic #10711

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jun 9, 2022

Overview

Found a silly little off-by-one error in the Discovery Client's robot removal logic while testing robot renames.

This can be a cause for duplicate robots remaining in the "unavailable" list with their old name after a rename, but the other cause is hard-to-avoid race with mDNS advertisements from the robot being renamed, so you probably won't notice much improvement after this PR. Still a bug though!

(mDNS advertisement race listed above is tracked by #10689)

Review requests

  • Code and test changes make sense

Risk assessment

Low.

@mcous mcous added the app Affects the `app` project label Jun 9, 2022
@codecov
Copy link

codecov bot commented Jun 9, 2022

Codecov Report

Merging #10711 (04ffed7) into edge (c11da0c) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##             edge   #10711    +/-   ##
========================================
  Coverage   73.82%   73.82%            
========================================
  Files        2152     2153     +1     
  Lines       58225    58362   +137     
  Branches     5961     6018    +57     
========================================
+ Hits        42983    43087   +104     
- Misses      13969    13987    +18     
- Partials     1273     1288    +15     
Flag Coverage Δ
app 71.61% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
discovery-client/src/store/reducer.ts 91.66% <0.00%> (ø)
...librationDetails/PipetteOffsetCalibrationItems.tsx 75.00% <0.00%> (-5.00%) ⬇️
...es/hooks/useRunStartedOrLegacySessionInProgress.ts 80.00% <0.00%> (ø)
...Devices/RobotSettings/RobotSettingsCalibration.tsx 70.45% <0.00%> (+2.45%) ⬆️
.../RobotSettings/CalibrationDetails/OverflowMenu.tsx 72.81% <0.00%> (+3.96%) ⬆️

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

¹

@mcous mcous merged commit 70cb3cb into edge Jun 10, 2022
@mcous mcous deleted the dc_fix-off-by-one-error branch June 10, 2022 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants