Conversation
fenhl
added
Status: Needs Review
Co-authored-by: Vincent Cunningham <flagrama@users.noreply.github.com>
cjohnson57
left a comment
cjohnson57
left a comment
There was a problem hiding this comment.
Might wanna reword this a bit... Maybe "testing it in-depth"? I think you should at least test your PR on a basic level before submitting it, like at least make sure it generates a seed successfully with your new setting or w/e, but of course we don't want to require the author to run through full seeds or anything like that.
|
I feel like that's going to depend a lot on the nature of the PR, and what sort of testing we want to see in addition to what's covered by CI (which a new setting generating successfully usually will be unless it's deliberately excluded from both Easy Mode and Hell Mode) might not always be obvious to the PR author. And having a strict requirement like that would discourage things like quick fixes written by one contributor that are tested by someone else. We can still request specific things to be tested in comments. |
|
Yeah I don't think we should try to define any hard rules, but I still think we should maybe say "without testing it in-depth" or "without heavily testing it". I don't know, it just feels kind of wrong to say yeah send us code without testing it at all lol |
3 participants
This adds a Markdown document that will automatically be provided as a starting point for writing the descriptions of new PRs. The current template is pretty minimal, only adding a “testing” section header as well as a couple comments with guidelines for writing the PR description. I had the idea to add this since we currently require testing for almost every PR, and it seems like most contributors do test their PRs, but they don't necessarily communicate that in the PR descriptions. My hope is that prompting people to report on what they've tested will allow us to merge PRs more quickly, but also help identify gaps in testing for some PRs.
Testing
The template was manually copied into this PR's description for a preview of what it looks like. I'd say testing whether the template is applied correctly to new PRs is best done on Dev, since PR templates are only active when they're on the main branch.