fix(app): Run timer stays at 00:00:00 if you reconnect in the middle of a delay #7937
Conversation
…of a delay: Starting over and taking the the option behind curtain number two in the PR to revise the way pause and runtime operate. Actually it's really well spelled out in the old PR so going to drop a variation of it in here: - Remove the runTime state entry because it represents "now, if running", and we can calculate that from startTime in a selector / in the UI component - Remove remoteTimeCompensation state entry and associated machinery, because with time syncing, this has been made unnecessary - Remove the TICK_RUN_TIME action and associated machinery, because why would we need to do this in state? - Total run time is now driven by: - Hasn't run yet: nothing, it's 0 - When running / paused: Date.now() less session.startTime - When stopped / errored / finished: session.statusInfo.changedAt (because that's when the status changed to stopped, etc.) - Current paused time is driven by: - If not paused: nothing, it's 0 - If paused: Date.now() less session.startTime + session.statusInfo.changedAt - If robot is paused, user knows clearly that it's paused and for how long it's been paused, because UI clearly communicates a ticking time labeled "Paused For" - tore out `remoteTimeCompensation` - tore out `TICK_RUN_TIME` (UI no longer updates. Need to fix) - some crumby styling
…edTime and trying to replication. I am settling down with what might be a unpalatable use of selectors and will run it by the community before testing. - revise timestamp dependent selectors to return a method that takes a timestamp. Motivations for doing so are commented within.
… Going to work on some flow -> TS tickets. - addressing some of the feedback on the draft.
- replace css components with basic UI atom components (see PR feedback) - manually and thoroughly test all scenarios - unit test changes
- cleanup clearRunTimerInterval
- pull in edge's changes to typeography.css
celsasser
changed the title
celsasser
changed the title
celsasser
force-pushed
the
fix-run-timer-revision
branch
from
0bd4924 to
788a83c
Compare
I have powers to destroy things that I wasn't even aware of... |
This is due to the macOS filesystem of some unknown number of GitHub Actions workers being crazy slow. It will cause our tests on macs flake out depending on what type of machine we get out of the macOS pool. If you see something failures in macOS that look completely unrelated to your code, re-run the job and see if you get a pass |
| it('displays the start time if not null', () => { | ||
| const { startTime } = mockEnvironment() | ||
| const { wrapper } = render() | ||
| const expected = format(startTime!, 'pp') |
There was a problem hiding this comment.
Oh oops, missed this one. Might make sense to pop formatTime out into a util, too
…onal `when` mock breaks the selectors with a second param which can be dealt with but just cannot see the value that it adds to this suite of tests.
| }: Partial<Environment> = {}): Environment { | ||
| when(getIsPausedMock).calledWith(MOCKED_STATE).mockReturnValue(isPaused) | ||
| when(getStartTimeMsMock).calledWith(MOCKED_STATE).mockReturnValue(startTime) | ||
| getPausedSecondsMock.mockReturnValue(pausedSeconds) |
There was a problem hiding this comment.
Rather than switch these to unconditional, I would have added the second argument to make sure they were getting the time correctly. As written, this does not ensure the component is properly collaborating with its dependencies:
when(getPausedSecondsMock)
.calledWith(MOCKED_STATE, expect.anby)
.mockReturnValue(pausedSeconds, expect.any(Number))Even this stub above isn't great; we have no evidence that we're passing in a good time. I would probably solve this by moving useTickingNow or something to its own dependency and mocking it out here, but for now I think that would be overkill and we should get this PR in.
Generally, I consider any use of unconditional return from stubs to be a sign of an insufficient test
There was a problem hiding this comment.
Your point is totally valid, but I was getting to the overkill conclusion. But it isn't much work to make it more robust.
Thank you! |
DRAFT
Will make sure tests are passing and all feedback from has been resolved
Overview
If the Opentrons App disconnects from an OT-2 in the middle of a protocol, and reconnects while the OT-2 is in the middle of a delay step, the Opentrons App's run timer will be stuck at 00:00:00, as if the protocol had never started.
This may contribute to users thinking a protocol has "canceled itself."
This PR has some history and has gone through some revisions and I have made a darn good mess of it. There are two previous PRs both with excellent feedback (and I am sorry about the forced close on the last one. I revised the commit history because I made a mess by merging in
edge)Changelog
runTimetimerReview requests
Please approve it :)
Risk assessment