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

updated CI to use latest version of this action when running tests #32

Merged
merged 4 commits into from
Nov 25, 2021

Conversation

stevengill
Copy link
Member

Summary

updated CI to use latest version of this action when running tests

Requirements (place an x in each [ ])

@stevengill stevengill self-assigned this Nov 24, 2021
@stevengill stevengill force-pushed the update-ci branch 4 times, most recently from f5cb74d to a9c1261 Compare November 24, 2021 19:25
@seratch seratch added this to the 1.17 milestone Nov 24, 2021
@stevengill stevengill force-pushed the update-ci branch 8 times, most recently from 67e2732 to 4027ce2 Compare November 24, 2021 23:51
@stevengill
Copy link
Member Author

Alright, this is ready for a review @filmaj @seratch

I broke out the botToken and webhook integration tests into their own jobs. Added a job for unit tests.

The webhook job does some normalizing of the github paylods (between push and pull request) and sends the normalized object to workflow builder. A pretty nice usecase to share I think.

We may want to add a unit test + integration test for incoming webhook as well here.

@stevengill
Copy link
Member Author

Also, @filmaj the unit test job outputs the entire payload of the event. I feel like this shouldn't be happening. See the npm test section in https://github.com/slackapi/slack-github-action/runs/4318381157?check_suite_focus=true

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

.github/workflows/main.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@filmaj filmaj left a comment

Choose a reason for hiding this comment

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

Hey this is awesome! Love the integration test jobs you wrote up, left a few comments on potential tweaks.

As for the unit test output, I believe this is happening because in one of the error paths in the code for the action, we console.log the entire payload, which itself may be loaded from a GitHub Action event (which furthermore comes in via the @actions/github module). So in our situation here: since the CI for this Action is itself running via GitHub Actions, the relevant GitHub-Actions-specific environment variables are present, so the pull request GitHub event payload info is loaded up when running the unit tests in CI 😆

We could comment out the console.log in the error path as one potential solution. Or leave it as-is. I don't think it's a big deal either way. The unit tests exercise that error path, so I think it is expected.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@stevengill stevengill merged commit ca00a9a into main Nov 25, 2021
@filmaj filmaj deleted the update-ci branch November 25, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants