Skip to content

Add slash command '/test-gmt-dev' to test GMT dev version #831

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

Merged
merged 7 commits into from
Feb 2, 2021

Conversation

seisman
Copy link
Member

@seisman seisman commented Feb 1, 2021

Description of proposed changes

This PR adds the slash command /test-gmt-dev which can trigger the GMT Dev Tests workflow. So we can test GMT master branch anytime, without requesting reviews from other maintainers or teams (Also fixes #828).

As you may know/remember, the slash command can only read the configurations in the master branch, so we can't test this PR unless we merge it into the master branch.

To test this PR, I forked the PyGMT repository to my own account (https://github.com/seisman/pygmt), and made changes directly to my own master branch.

If you compare my master branch and the PyGMT master branch, you will see my master branch is 2 commits ahead of PyGMT master branch. The two commits are:

  • seisman@11446e2: This commit is the same as the only commit in this PR
  • seisman@37f83e4: The slash command uses the GenericMappingTools bot to generate a token, which is not available in my own fork. So I have to use my own token instead. This commit is necessary in my own fork for testing purposes.

To confirm that everything works well, I opened an example PR in my own fork (https://github.com/seisman/pygmt/pull/2). Please go to that PR to see how it works.

TODO:

Known issues:

The "GMT Latest Tests" triggered by /test-gmt-master won't show the workflow status in the status summary page (like the screenshot below), so we have to go the the "Actions" page to find the testing results.
image

Note: you may find the following status page in PR https://github.com/seisman/pygmt/pull/2. These "GMT Latest Tests" status were triggered by "mark it ready for review", not by the slash command /test-gmt-master.
image

One possible solution is: when the workflow is triggered by slash commands, we can let the workflow leave a comment saying that "The workflow is running at https://github.com/GenericMappingTools/pygmt/actions/runs/XXXXX"

Fixes #828.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Notes

  • You can write /format in the first line of a comment to lint the code automatically

@seisman seisman added the maintenance Boring but important stuff for the core devs label Feb 1, 2021
@seisman seisman added this to the 0.3.0 milestone Feb 1, 2021
@seisman seisman requested a review from weiji14 February 1, 2021 06:55
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Please also update the part in https://github.com/GenericMappingTools/pygmt/blob/v0.2.1/MAINTENANCE.md#github-actions which says:

ci_tests_dev.yaml (GMT Latest Tests on Linux/macOS/Windows).

This is only triggered when a review is requested or re-requested on a PR. It is also scheduled to run daily on the master branch.

to mention that the test can also be triggered using /test-gmt-master.

Note: you may find the following status page in PR seisman#2. These "GMT Latest Tests" status were triggered by "mark it ready for review", not by the slash command /test-gmt-master.
image
One possible solution is: when the workflow is triggered by slash commands, we can let the workflow leave a comment saying that "The workflow is running at https://github.com/GenericMappingTools/pygmt/actions/runs/XXXXX"

Might be complicated trying to code up the chatbot comment (though it will be nice, if not a bit annoying). How often will we need to keep triggering GMT Latest Tests? If it's not too often I think it's fine to just navigate to the Actions tab manually.

@seisman
Copy link
Member Author

seisman commented Feb 1, 2021

Might be complicated trying to code up the chatbot comment (though it will be nice, if not a bit annoying). How often will we need to keep triggering GMT Latest Tests? If it's not too often I think it's fine to just navigate to the Actions tab manually.

We probably only want to trigger the GMT Latest Tests when a PR is close to merging. We can navigate to the Actions tab manually and see how it goes.

@seisman seisman requested a review from weiji14 February 1, 2021 21:03
@@ -21,5 +21,6 @@ jobs:
token: ${{ steps.generate-token.outputs.token }}
commands: |
format
test-gmt-master
Copy link
Member

@weiji14 weiji14 Feb 1, 2021

Choose a reason for hiding this comment

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

Suggested change
test-gmt-master
test-gmt-dev

What do you think about changing the name to 'latest' to match the Github Actions name?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we say "the latest GMT", we usually mean "the latest stable GMT release" (i.e., 6.1.1).

What about "GMT Dev Tests" and /test-gmt-dev?

Copy link
Member

@weiji14 weiji14 Feb 1, 2021

Choose a reason for hiding this comment

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

Yep, dev sounds good and is shorter. Please remember to change the PR title too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 6df8e34 and 72b7dfa

@seisman seisman changed the title Add slash command '/test-gmt-master' to test GMT master branch Add slash command '/test-gmt-dev' to test GMT master branch Feb 1, 2021
Co-authored-by: Wei Ji <[email protected]>

Co-authored-by: Wei Ji <[email protected]>
@seisman seisman changed the title Add slash command '/test-gmt-dev' to test GMT master branch Add slash command '/test-gmt-dev' to test GMT dev version Feb 2, 2021
@seisman seisman merged commit 213a414 into master Feb 2, 2021
@seisman seisman deleted the slash-test-gmt branch February 2, 2021 00:31
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…pingTools#831)

* Add slash command '/test-gmt-dev' to test GMT dev version
* Remove 'review_requested' from the triggering condition
* Update pull request template
* Update maintenance guide
* Rename workflow to 'GMT Dev Tests'

Co-authored-by: Wei Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The workflow of "GMT Latest Tests" are triggered multiple times
2 participants