-
Notifications
You must be signed in to change notification settings - Fork 0
Added deployment unit and integration testing #16
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
Conversation
Jest ResultsDuration: 1.392 seconds
Outcome: Passed | Total Tests: 11 | Passed: 11 | Failed: 0
|
Before this PR can be merged, the following item(s) should be addressed to comply with the action's Contributing Guidelines.
|
The action's Contributing Guidelines have been met:
|
Before this PR can be merged, the following item(s) should be addressed to comply with the action's Contributing Guidelines.
|
The action's Contributing Guidelines have been met:
|
const createOctokitClient = token => new Octokit({ auth: token }); | ||
const createOctokitGraphQLClient = token => | ||
graphql.defaults({ headers: { authorization: `token ${token}` } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an advantage to defining a function that creates an octokit client as opposed to creating a single client that is passed to the functions that need it? It seems like having a single client that all the different functions use might be better than each function creating its own version of the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is getting used by the deployment testing file, too. I wanted to make sure the setting params for these clients were the same in both the runtime code as well as the jest test.
src/library.test.js
Outdated
status => { | ||
process.env[inputKey('deployment-status')] = status; | ||
try { | ||
const testConext = setup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nitpick but it looks like there is a typo here. testConext
=> testContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
The action's Contributing Guidelines have been met:
|
The action's Contributing Guidelines have been met:
|
Summary of PR changes
PR Requirements
+semver:
keywords.NOTE: If the repo's workflow could not automatically update the
README.md
, it should be updated manually with the next version. For javascript actions, if the repo's workflow could not automatically recompile the action it should also be updated manually as part of the PR.