-
Notifications
You must be signed in to change notification settings - Fork 24
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
Regex Unit Testing, Refactoring, and Documentation Cleanup #111
Open
EugeneWilson
wants to merge
15
commits into
junlarsen:master
Choose a base branch
from
EugeneWilson:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
Changed import to only use one function instead of entire module
Added ProcessArgsParsingError
Also added type checking and implemented the new error class
Created new function getProcessArgs
Attempting to make certificate ternary easier to read and updating docs
Fix undefined regex when argument is at end of string.
Removed unnecessary else, added pollInterval, updated resolve
Added LeagueWebSocketInitError
Made errorHandler it's own function and added ErrorCode enum
Added multiple new tests, removed unused vars, updated docs.
This is huge, thank you! I'll review it once I have time, hopefully this weekend. |
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.
Description
This PR intends to add unit testing, clean up some documentation, make debugging easier for users, and improve code readability in certain areas. It was important when making these changes to make sure that none of these changes would break the existing code that users have written while using this library, and I believe I've succeeded with that. There are a lot of changes, so I'd love to hear your feedback. I tried to make sure that I documented all the changes in this PR, and I attempted to break it up into multiple small commits to make changes easier to review. However, if I have missed anything or if you have any questions, please let me know.
Changes
Authentication.ts
exec
andpromisfy
functions instead of the entire modules. This should make the code a bit more efficient and easier to read.exec
to match the new imports.ProcessArgsParsingError
class for when there's an issue with the regex matching. Included publicrawStdout
,port
,password
, andpid
properties to the class for easier debugging.tryAuthenticate()
returns a promise that resolves to theCredentials
object.authenticate()
.getProcessArgs()
andparseProcessArgs()
. Both functions are exported; however, they are marked with@internal
and are not exported inindex.ts
, so TypeScript won't export their definitions.parseProcessArgs()
rawStdout
, which should contain the command line arguments;unsafe
, a boolean that determines if the user wants to connect in unsafe mode (with an undefined certificate), which defaults to false; andcertificate
, an optional string that contains a user provided certificate.|$
to the end of each regex to ensure that if the port, password, or pid is the last argument, it still matches properly.ProcessArgsParsingError
error.Authentication.test.ts
getProcessArgs()
function and regex inside of it works as expected. Those tests use different stdout formats that have been seen in issues. The new tests are:Note:
PLAINTEXT_CERT
doesn't perfectly match the default certificate as it's missing a new line at the beginning. I'm unsure if this was intentional, but I used that difference to do testing with a different certificate.Webhook.ts
ErrorCode
enum. This is used by the error handler to relay what type of error occurred and what action to take.options.__internalMockFaultyConnection
as it is no longer needed for testing.LeagueWebSocketInitError
class for when there's an issue initializing the websocket. The original error event is passed using theerrorEvent
property. Although this is a long class name, I felt it was important to clarify that this error happens during initialization and not after it's connected.errorHandler()
, to assist in easier testing. This function is exported; however, it is also marked with@internal
, so TypeScript won't export it's definition.errorHandler()
has two arguments for the function.err
accepts the error thrown by the webhook andoptions
which is the options the user provided when creating the webhook. Returns a ErrorCode enum value.ws.onerror
was updated to call the newerrorHandler()
function and pass the error and options. The return value is then used to determine what action to take.Webhook.test.ts
MockWebSocket
class that extends theEventEmitter
class. This class is used to mock the websocket connection errors.maxRetries
set to 0.maxRetries
set to 3 andpollInterval
set to 500. This test is truly run 4 times as the first attempt is not considered a retry.Unknown
is used for this test, but it could be any error.__internalMockFaultyConnection
was removed.ws.onerror
part of the code works as expected.Client.test.ts
index.ts
ClientElevatedPermsError
class.ProcessArgsParsingError
andLeagueWebSocketInitError
to the export.