-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add documentation: testing a pull request #1901
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
doc/source/testing_pull_requests.rst
Outdated
middle of the line? that should be changed as needed for each pull | ||
request that you wanna to test | ||
- `p4a-feature-fix-numpy`: the name of the cloned repository, so we can | ||
have multiple clones of different prs in the same folder |
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.
I actually assumed this part is known by contributors, so I would also have skipped that part. But it's good that you have it there then.
There's also a "shortcut" to actually fetch the PR given its number:
git fetch origin pull/<#>/head:<local_branch_name
For instance for yours:
git fetch upstream pull/1901/head:feature-docs-test-pr
remote: Enumerating objects: 6, done.
remote: Counting objects: 100% (6/6), done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 6 (delta 4), reused 6 (delta 4), pack-reused 0
Unpacking objects: 100% (6/6), done.
From https://github.com/kivy/python-for-android
* [new ref] refs/pull/1901/head -> feature-docs-test-pr
[git checkout feature-docs-test-pr
Switched to branch 'feature-docs-test-pr'
The workflow I personally use most of the time is to add the remote and checkout the branch. So for yours:
git remote add opacam https://github.com/opacam/python-for-android.git
git fetch opacam
git checkout opacam/feature-docs-test-pr
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.
Oh yeah, I will write that 😉
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.
Great initiative 👏
A couple of suggestions, but I don't know if it's out of the scope.
In the big picture, thing and step I take while reviewing a PR.
- of course checking if the overall thing makes sense
- is the CI passing, if not what specifically fails
- is it working locally at compile time
- is it working on device at runtime
Anyway, nice work! I don't think we have to wait to get it perfect before merging. The document can evolve in later iterations.
67baa0a
to
e8a8c13
Compare
e8a8c13
to
f5e78b7
Compare
Is this still WIP? For me it's OK for a merge 👍 |
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.
Nice documentation, thank you
Due to the lack of documentation on how to test a pull request, sometimes we are enforced to fill pull requests with support comments about how to do the testing or we directly don't answer (to not fill the pr with unrelated information), yeah...I'm guilty of that in #1722...sorry ☮️ . So to remediate this issue, I created this pull request to solve this once and for all, but guys, I need your help in here, because each one of us can have his own method, so it would be great to share our testing methods and write them in the docs, for future reference and to redirect users in case that we need to do so with a minimal interference possible for the affected pull request.
I put three methods in the initial documentation, but I need that all of you review this very carefully because I could miss something (actually I think that I'm missing something...) or it may exist a better ways to accomplish this...probably...which I'll be happy to learn 😄...so I labeled this as WIP, until we make sure that this documentation is well enough to be published and contemplates the best methods to do the
pull requests
testing