Skip to content

Commit 617f742

Browse files
chore: PR template and validation updates (#3582)
## Purpose of this PR This PR aims to increase the quality of our PRs by enforcing certain considerations and setting examples via PR template. This will also help with Playtesting and validation of our changes by providing a better way to evaluate PRs for potential playtest specification or release blocking. It is similar to an "official" suggestion from https://github.com/orgs/community/discussions/84771 New PR template will be working as a "guide" on what would be nice to have in it but the content in the template is more of an example. What matters are sections: 1. **Documentation** --> Should describe what documentation changes/corrections followed the code changes. This aims to help catching any missing docs but if no documentation changes (API docs, Documentation~) were needed then you can just acknowledge this fact in this section 2. **Testing and QA** --> Should explain what testing coverage was added for the code changes. This helps to understand if a given change will require playtesting or is it fully covered by automated tests. This can range from "a new sample was created with..." to "edge case issue that is covered by added automated test". Note that **WE SHOULD ALMOST ALWAYS** add some form of testing. The proposed content of this section helps to see if you performed some manual testing (if you created some project it's always worth linking but no need), it also helps with with general evaluation of changes. 3. **Backport** --> A section that we already have. Should enforce consideration if those changes should be ported to `develop`/`develop-2.0.0` branches respectively 4. **Jira ticket** section was added for consideration if such PR is big enough to require Jira ticket present. Note that this section is not mandatory but may be flagged by QA when evaluating PRs for release. I would assume that if PR takes more than a day of work then it should have a corresponding Jira ticket Those sections make it WAY easier to evaluate PRs that landed when we want to release the package. It allows to see developer thinking on important areas and works as a reminder for them so in case of looking at the PRs it's easier for the QA to analyze those. Important to note are : - PR template was updated with more sections where **Documentation**, **Testing and QA**, and **Backport** sections are **REQUIRED** for a PR to be merged. - I renamed the git workflow and I will update the name in the settings when merging this PR so for now you can see backport-verification pending since it doesn't exist anymore. - **Testing and Documentation** section was basically divided into separate **Documentation** and **Testing and QA sections** Such updated PR template will look as follows <img width="1388" height="1855" alt="image" src="https://github.com/user-attachments/assets/7fe2f129-0440-48c3-98a3-b0eb9bae8e62" /> ### Jira ticket MTT-12822 ### Documentation No documentation changes or additions were necessary. ## Testing & QA This change won't affect users and focuses around our workflow and templates so no testing is needed ## Backport #3583
1 parent f4b6cd4 commit 617f742

File tree

3 files changed

+110
-55
lines changed

3 files changed

+110
-55
lines changed

.github/pull_request_template.md

Lines changed: 48 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,61 @@
1-
<!-- Replace this block with what this PR does and why. Describe what you'd like reviewers to know, how you applied the engineering principles, and any interesting tradeoffs made. Delete bullet points below that don't apply, and update the changelog section as appropriate. -->
1+
## Purpose of this PR
2+
[//]: # (
3+
Replace this block with what this PR does and why. Describe what you'd like reviewers to know, how you applied the engineering principles, and any interesting tradeoffs made.
4+
)
25

3-
<!-- Add short version of the JIRA ticket to the PR title (e.g. "feat: new shiny feature [MTT-123]") -->
6+
### Jira ticket
7+
_Link to related jira ticket ([Use the smart commits](https://support.atlassian.com/bitbucket-cloud/docs/use-smart-commits/))_
48

5-
## Changelog
9+
### Changelog
10+
[//]: # (updated with all public facing changes - API changes, UI/UX changes, behaviour changes, bug fixes. Remove if not relevant.)
611

712
- Added: The package whose Changelog should be added to should be in the header. Delete the changelog section entirely if it's not needed.
8-
- Fixed: If you update multiple packages, create a new section with a new header for the other package.
9-
- Removed/Deprecated/Changed: Each bullet should be prefixed with Added, Fixed, Removed, Deprecated, or Changed to indicate where the entry should go.
10-
11-
## Testing and Documentation
12-
13-
- No tests have been added.
14-
- Includes unit tests.
15-
- Includes integration tests.
16-
- No documentation changes or additions were necessary.
17-
- Includes documentation for previously-undocumented public API entry points.
18-
- Includes edits to existing public API documentation.
13+
- Fixed: If you update multiple packages, create a new section with a new header for the other package.
14+
- Removed/Deprecated/Changed: Each bullet should be prefixed with Added, Fixed, Removed, Deprecated, or Changed to indicate where the entry should go
1915

2016
<!-- Uncomment and mark items off with a * if this PR deprecates any API:
2117
### Deprecated API
2218
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter yyyy-mm-dd)` entry.
23-
- [ ] An [api updater] was added.
19+
- [ ] An [api updater](https://confluence.unity3d.com/display/DEV/Obsolete+API+updaters) was added.
2420
- [ ] Deprecation of the API is explained in the CHANGELOG.
2521
- [ ] The users can understand why this API was removed and what they should use instead.
2622
-->
2723

28-
## Backport
24+
## Documentation
25+
[//]: # (
26+
This section is REQUIRED and should mention what documentation changes were following the changes in this PR.
27+
We should always evaluate if the changes in this PR require any documentation changes.
28+
)
2929

30-
<!-- If this is a backport:
31-
- Add the following to the PR title: "\[Backport\] ..." .
32-
- Link to the original PR.
33-
If this needs a backport - state this here
34-
If a backport is not needed please provide the reason why.
35-
If the "Backports" section is not present it will lead to a CI test failure.
36-
-->
30+
- No documentation changes or additions were necessary.
31+
- Includes documentation for previously-undocumented public API entry points.
32+
- Includes edits to existing public API documentation.
33+
34+
## Testing & QA
35+
[//]: # (
36+
This section is REQUIRED and should describe how the changes were tested and how should they be tested when Playtesting for the release.
37+
It can range from "edge case covered by unit tests" to "manual testing required and new sample was added".
38+
Expectation is that PR creator does some manual testing and provides a summary of it here.)
39+
40+
### Functional Testing
41+
[//]: # (If checked, List manual tests that have been performed.)
42+
_Manual testing :_
43+
- [ ] `Manual testing done`
44+
45+
_Automated tests:_
46+
- [ ] `Covered by existing automated tests`
47+
- [ ] `Covered by new automated tests`
48+
49+
_Does the change require QA team to:_
50+
51+
- [ ] `Review automated tests`?
52+
- [ ] `Execute manual tests`?
53+
54+
If any boxes above are checked, please add QA as a PR reviewer.
55+
56+
## Backport
57+
[//]: # (
58+
This section is REQUIRED and should link to the PR that targets other NGO version which is either develop or develop-2.0.0 branch
59+
Add the following to the PR title: "\[Backport\] ..."
60+
If this is not needed, for example feature specific to NGOv2.X, then just mention this fact.
61+
)

.github/workflows/backport-verification.yml

Lines changed: 0 additions & 32 deletions
This file was deleted.

.github/workflows/pr-verification.yml

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# This workflow is designed to verify that the pull request description contains a required sections that are important from quality perspective.
2+
# ## Backport section is important as a reminder to account for backports for anyone that works with NGO repository (to 1.X or 2.X branches respectively).
3+
# ## Testing & QA section is important to ensure that the PR has appropriate testing coverage and is important when QA will evaluate PRs before Playtesting for the release.
4+
# ## Documentation section is important to ensure that the documentation is updated with the changes made in the PR.
5+
6+
# If any of the sections is missing, the workflow will fail and block the PR from merging, prompting the developer to add those sections to the PR description.
7+
# The workflow is configured to run when PR is created as well as when it is edited which also counts simple description edits.
8+
9+
name: "NGO - PR Verification"
10+
11+
on:
12+
pull_request:
13+
types: [opened, edited, synchronize, reopened]
14+
branches:
15+
- "develop"
16+
- "develop-2.0.0"
17+
- "/release\/.*/"
18+
19+
jobs:
20+
pr-verification:
21+
runs-on: ubuntu-latest
22+
steps:
23+
- name: Checkout code
24+
uses: actions/checkout@v4
25+
26+
- name: Check PR description
27+
uses: actions/github-script@v7
28+
with:
29+
script: |
30+
const pr = context.payload.pull_request;
31+
const body = pr.body || '';
32+
33+
// List of mandatory PR sections
34+
const requiredSections = [
35+
{
36+
header: '## Backport',
37+
description: 'PR description must include a "## Backport" section. Please add this section and provide information about this PR backport to develop or develop-2.0.0 branch respectively or explain why backport is not needed.'
38+
},
39+
{
40+
header: '## Testing & QA',
41+
description: 'PR description must include a "## Testing & QA" section. Please add this section and provide information about the testing performed for this PR. It can range from adding unit tests to full samples and is needed from QA side to analyze PRs while Playtesting for the release.'
42+
},
43+
{
44+
header: '## Documentation',
45+
description: 'PR description must include a "## Documentation" section. Please add this section and provide information about the documentation changes made in this PR. It is important to keep the documentation up to date with the code changes.'
46+
}
47+
];
48+
49+
const missing = requiredSections.filter(section => !body.includes(section.header));
50+
51+
if (missing.length > 0) {
52+
let message = 'PR description is missing the following required section(s):\n';
53+
54+
const missingDescriptions = missing.map(
55+
s => `- ${s.header}: ${s.description}`
56+
);
57+
58+
message += missingDescriptions.join('\n');
59+
message += '\n\nPlease add them to your PR description.';
60+
61+
core.setFailed(message);
62+
}

0 commit comments

Comments
 (0)