fix: reset ready timeout on successful connection #160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Main issues are fixed. Working on how we might leverage the extension test api to test different aspects of the duplex stream on the ssh connection provider to shore up some coverage gaps. |
* add sinon types to aid with type safety * refactor contentdataprovider tests to use types * add ts-sinon library to add with stubbing of interfaces
Becky-Williamson
added
testing
scottdover
reviewed
Mar 22, 2023
client/src/connection/ssh/index.ts
Outdated
| stream?.write(`%put ${endCode};\n`); | ||
| }); | ||
| } | ||
| setTimer() { |
There was a problem hiding this comment.
Can we add method private/protected/etc to these methods to get a better idea of what's publicly accessible?
There was a problem hiding this comment.
Good catch, meant to do this as part of moving the logic into a class definition. Will add these.
* use arrow functions * visibility modifiers on class functions * refactor listeners into discreet methods
scottdover
reviewed
Mar 23, 2023
Co-authored-by: Scott Dover <scott@scottdover.com>
Becky-Williamson
added
testing-complete
Co-authored-by: Scott Dover <scott@scottdover.com>
scottdover
approved these changes
Mar 23, 2023
Becky-Williamson
added
verified
and removed
testing-complete
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Correct the connection ready timeout to clear when a successful connection is made. The previous behavior was prematurely closing the ssh session because the timeout was firing even after a successful connection ready event was encountered. In the timeout function there is code to gracefully end the connection.
This behavior resulted in subsequent run button presses resetting/re-establishing the sas -nodms interactive session. When this happened any intermediate work table content was lost.
Testing (Developer)
Test via steps to reproduce:
1. Connect to a 9.4 environment with a SAS 9.4 Remote connection profile.
2. Place the following sas code in the editor contents:
data test; set sashelp.class; run;3. Click run. Verify that no errors appear in the log.
4. Replace editor contents with code that uses the work table:
proc print data=test; run;5. Click run.
6. Verify that proc print step from 4 prints the results from the work table to the log.
7. Verify that result tab shows with 19 observations.
8. Click the close button.
9. Replace the editor contents with code from step 4.
10. Click run.
11. Verify that the intermediate work table is no longer found with error:
File WORK.TEST.DATA does not exist.Test that a sas program that runs for longer than the ready timeout (default 10) seconds value does not prematurely timeout. Program code:
data _null_; slept=sleep(25,1); run;Add appropriate unit tests