Improve the pull request template

[Calinou]

This guides people towards better describing what their pull request does and how much they've tested it.

In addition, it highlights the recommendation to write unit tests in pull requests.

Inspired by the template here. Thanks @bruvzg 🙂

[neikeq]

This looks good if we want to make tests mandatory. Otherwise I think it could confuse contributors making them think that's the case. Maybe this should be clarified in the comment.

[aaronfranke]

Considering the vast majority of the codebase doesn't have unit tests yet, it seems a bit unfair to ask users to do this. Maybe we could add this checkbox to the template later, but for now we would only have the first two.

[CitrusWire]

Considering the vast majority of the codebase doesn't have unit tests yet, it seems a bit unfair to ask users to do this. Maybe we could add this checkbox to the template later, but for now we would only have the first two.

But you've got to start adding Unit Tests somewhere, and lets be honest, no-ones going to be taking on writing large amounts of unit tests against the current codebase any time soon.

[Calinou]

I would prefer keeping this recommendation, especially now that #42430 has been merged.

I think the comment makes it obvious enough already that it's not a strict requirement.

[Ansraer]

We might want to reconsider if/where we want to make unit tests mandatory once we have published 4.0 and no longer have such massive changes, but this looks good for right now.

[akien-mga]

Should we revisit this with the new form template?

[Calinou]

Should we revisit this with the new form template?

GitHub doesn't appear to support issue forms for pull requests. I can't see any mention of it in the documentation: https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms

[Calinou]

Bump 🙂

There was some concern about this pull request causing every other PR opened to have checklist mode enabled on the list of pull requests.

However, it looks like checklist display can be prevented by putting all checkboxes within a quote. It also deemphasizes the text itself to look less intrusive. See Calinou/scoop-games#637 for an example of this, as well as the PR template used.

If you look at the list of pull requests, the PR doesn't appear to be a checklist despite containing checkboxes:

I've amended this pull request to use this technique.

[akien-mga]

I'm still not convinced by this change, because IMO it makes everything too verbose. There's too much content to read, and most of it is only relevant for new contributors to read the first time - for existing contributors who might make several PRs on a daily basis, I fear this is just going to get in the way (like the current template does, where we mostly end up Ctrl+A and Ctrl+X deleting it).

I made a test on a repo with the proposed template here, here's how it looks like when opening a new PR in Firefox:

I highlighted the scrollbar so you can see that by default the text area fits only 1/4th of the template. So there's a lot of scrolling involved to get to all fields, and personally it makes me feel claustrophobic.

If I want to actually follow the template to provide a nicely formatted PR with a significant amount of details, then I'll also need to delete the HTML comments manually because they get in my way and prevent me from getting a good overview of my own writing. Otherwise, it looks like this and it's pretty hard to proofread myself and do further edits when needed:

simplescreenrecorder-2022-06-24_11.56.40.mp4

---

So for this kind of template to really work without going in our way, we really need issue forms IMO so that all the information is provided in the form and not in the body of the template itself. This was a big annoyance when we had complex issue templates without the form in the past, and I'm not so keen on going back to the same issue for PRs.

I'm OK with changing the template to make some things clearer (and I'd likely get rid of the current paragraph because it's a hassle and people still make PRs against 3.4 anyway...), but IMO this would need to be heavily slimmed down. Everything which is meant to be read once and then kept in mind for future PRs should just go to CONTRIBUTING.md.

[CitrusWire]

At the risk of taking this in a slightly different direction:

What is the value of asking people to describe how they've tested things? Specifically, in contrast to requiring actual unit/integration tests instead?

Because if they've actually tested things thoroughly (as they should) and manually, and then have to write a "report" in the form of filling in this template, surely that same time could have been spent on implementing unit/integration tests? Said tests wouldn't require extensive reports (or any report at all), and would be repeatable by everyone in all environments. And you get the advantage of fewer regressions (currently showing over 2100 issues marked "regression"! :-o - that's a huge amount of dev time lost!) ) and greater test coverage.

Summary: To me it seems like this is solving the poor-testing problem in the wrong way.

[Calinou]

What is the value of asking people to describe how they've tested things? Specifically, in contrast to requiring actual unit/integration tests instead?

This is implicitly asking people to upload a testing project for their pull request, so we can confirm that their PR actually does what it's supposed to 🙂

I've seen too many PRs that were complex to test but had no reproduction project attached.

Because if they've actually tested things thoroughly (as they should) and manually, and then have to write a "report" in the form of filling in this template, surely that same time could have been spent on implementing unit/integration tests?

Not everything is currently unit/integration testable in Godot, and improving this can take too much time/effort to be worth it (at least in the short term).

[Calinou]

Superseded by #62380.