Skip to content

Link to Contributor's Guide from README + expand Maintainer's Guide#1071

Merged
seratch merged 2 commits intomainfrom
readme-maintainers
Jul 28, 2021
Merged

Link to Contributor's Guide from README + expand Maintainer's Guide#1071
seratch merged 2 commits intomainfrom
readme-maintainers

Conversation

@filmaj
Copy link
Copy Markdown
Contributor

@filmaj filmaj commented Jul 27, 2021

Summary

This PR fixes #1070 by:

  • linking to the Contributor's Guide from the README
  • adding more details about the virtual env setup to the Maintainer's Guide
  • mirroring the CI execution steps as closely as possible in the Maintainer's Guide
  • removed travis.yml and all references to travis from the Maintainer's Guide

Category (place an x in each of the [ ])

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./docs.sh?)
  • /docs-src-v2 (Documents, have you run ./docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

Link to Contributor Guide from README. Expand Maintainer Guide with m…
146ab77 
…ore details on virtual env setup. Mirror Maintainer Guide steps as closely to CI execution as possible. (#1070)
@filmaj filmaj requested review from seratch and stevengill July 27, 2021 16:46
@gitwave gitwave bot added the untriaged label Jul 27, 2021
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 27, 2021 •
edited
Loading

Codecov Report

Merging #1071 (14c5208) into main (8bbb5d4) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1071   +/-   ##
=======================================
  Coverage   84.14%   84.14%           
=======================================
  Files          99       99           
  Lines        9239     9239           
=======================================
  Hits         7774     7774           
  Misses       1465     1465

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bbb5d4...14c5208. Read the comment docs.

stevengill
stevengill approved these changes Jul 27, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@stevengill stevengill left a comment

Choose a reason for hiding this comment

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

LGTM!

.github/contributing.md

- [x] You must have signed the [Contributor License Agreement (CLA)](https://cla-assistant.io/slackapi/python-slack-sdk).
- [x] The test suite must be complete and pass.
- [x] The test suite must be complete and pass (see the [Maintainer's Guide](./maintainers_guide.md) for details on how to run the tests).
Copy link
Copy Markdown
Member

@stevengill stevengill Jul 27, 2021

Choose a reason for hiding this comment

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

Haha I see you did do this (i just mentioned it as a comment on the issue thread)

.github/maintainers_guide.md Outdated

```bash
python setup.py validate # run all
PYTHON_SLACK_SDK_MOCK_SERVER_MODE="threading" CI_UNSTABLE_TESTS_SKIP_ENABLED="1" python setup.py validate # run all
Copy link
Copy Markdown
Member

@stevengill stevengill Jul 27, 2021

Choose a reason for hiding this comment

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

Did you pull this from the github action?

Copy link
Copy Markdown
Contributor Author

@filmaj filmaj Jul 27, 2021

Choose a reason for hiding this comment

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

Indeed

Copy link
Copy Markdown
Contributor Author

@filmaj filmaj Jul 27, 2021

Choose a reason for hiding this comment

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

Without these env vars the tests fail for me locally

Copy link
Copy Markdown
Contributor

@seratch seratch Jul 27, 2021

Choose a reason for hiding this comment

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

What error do you face? CI_UNSTABLE_TESTS_SKIP_ENABLED is a workaround only for GitHub Actions env (I was not able to find any solution for it at the time). I don't recommend enabling this for local development. If the error is 100% reproducible on your local env, we should fix the issue.

Copy link
Copy Markdown
Contributor Author

@filmaj filmaj Jul 27, 2021

Choose a reason for hiding this comment

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

OK, I will revert this change in the docs then. Should I file a separate issue for test failures? The short summary is that the tests remain stuck at this point of the output if I run without these env vars:

tests/slack_sdk/socket_mode/test_interactions_builtin.py .F                       [ 47%]
tests/slack_sdk/socket_mode/test_interactions_websocket_client.py

Copy link
Copy Markdown
Contributor Author

@filmaj filmaj Jul 27, 2021

Choose a reason for hiding this comment

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

I can run the tests individually / separately per file to collect the log output, too, but perhaps a separate issue would be a better venue for that.

Copy link
Copy Markdown
Contributor

@seratch seratch Jul 27, 2021

Choose a reason for hiding this comment

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

@filmaj For the failure, if it's the same with #808, we can reopen it (I know the reset by peer errors still occasionally occur). For the stuck test, can you check logs/pytest.log to see the progress? The Socket Mode interactions tests tend to take much longer than others.

Copy link
Copy Markdown
Contributor Author

@filmaj filmaj Jul 27, 2021

Choose a reason for hiding this comment

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

It may be that... I do see connection reset by peer (also Broken Pipe error). Full log here.

As for the stuck test_interactions_websocket_client.py test, the logs/pytest.log seems to repeat a ConnectionRefusedError forever (full log here)

Copy link
Copy Markdown
Contributor

@seratch seratch Jul 28, 2021

Choose a reason for hiding this comment

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

@filmaj I've sent a pull request fixing this issue. Thanks for pointing it out! #1072

@stevengill stevengill added docs M-T: Documentation work only and removed untriaged labels Jul 27, 2021
@stevengill stevengill added this to the 3.9.0 milestone Jul 27, 2021
seratch
seratch suggested changes Jul 27, 2021
View reviewed changes
.github/maintainers_guide.md Outdated

```bash
python setup.py validate # run all
PYTHON_SLACK_SDK_MOCK_SERVER_MODE="threading" CI_UNSTABLE_TESTS_SKIP_ENABLED="1" python setup.py validate # run all
Copy link
Copy Markdown
Contributor

@seratch seratch Jul 27, 2021

Choose a reason for hiding this comment

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

What error do you face? CI_UNSTABLE_TESTS_SKIP_ENABLED is a workaround only for GitHub Actions env (I was not able to find any solution for it at the time). I don't recommend enabling this for local development. If the error is 100% reproducible on your local env, we should fix the issue.

@seratch seratch mentioned this pull request Jul 28, 2021
Merged
16 tasks
@seratch seratch merged commit 6f369e4 into main Jul 28, 2021
@seratch seratch deleted the readme-maintainers branch July 28, 2021 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevengill stevengill stevengill approved these changes

+1 more reviewer

@seratch seratch seratch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs M-T: Documentation work only

Projects

None yet

Milestone

3.9.0

Development

Successfully merging this pull request may close these issues.

Add instructions on how to set up for development / how to run tests locally

3 participants

@filmaj @seratch @stevengill