Skip to content
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

[DOC] Update pull request template to ask whether new tests have been added #4066

Closed
achieveordie opened this issue Jan 5, 2023 · 3 comments
Assignees
Labels
documentation Documentation & tutorials

Comments

@achieveordie
Copy link
Collaborator

Describe the issue linked to the documentation

I suggest we update sktime/PULL_REQUEST_TEMPLATE.md and add another subsection asking the author if they have added any new tests. This may be more important for bug-fixing PRs but is also relevant to PRs related to code enhancements.

It is considered good practice to add tests with newly added code to enforce the fact that the code actually works. This will reduce the chance of introducing logical bugs which often escape tests that only check for spec compliance

This line will serve as a cue for the author to add tests to ensure that further changes to these files won't introduce the same kind of bug.

Suggest a potential alternative/fix

#### Does your contribution introduce a new dependency? If yes, which one?

<!--
If your contribution does add a new hard dependency, we may suggest to initially develop your contribution in a separate companion package in https://github.com/sktime/ to keep external dependencies of the core sktime package to a minimum.
-->

#### Did you add any tests?

<! -- It is considered good practice to add tests whenever a new piece of code is added. While we don't mandate them, we encourage our contributors to add new tests to reduce the chance of introducing logical bugs. -->

#### What should a reviewer concentrate their feedback on?

<!-- This section is particularly useful if you have a pull request that is still in development. You can guide the reviews to focus on the parts that are ready for their comments. We suggest using bullets (indicated by * or -) and filled checkboxes [x] here -->
@achieveordie achieveordie added the documentation Documentation & tutorials label Jan 5, 2023
@achieveordie achieveordie self-assigned this Jan 5, 2023
@achieveordie
Copy link
Collaborator Author

@miraep8 @fkiraly @GuzalBulatova

What do you think?

@fkiraly
Copy link
Collaborator

fkiraly commented Jan 5, 2023

@achieveordie, good spot! We did not ask for this already in the current template? Apparently not!

Initially, I thought maybe it is better to modify it a little, to sth similar to "is the change covered by tests?" or "is there a test certifying for the change" - but there may be a better formulation than that. Since automated tests may already cover the change.

But, on second thought, perhaps "did you add any tests" is clearest - since this is mostly the requirement for beginneres, and the advanced contributors will know what to do in the less frequent cases that are different.

@miraep8
Copy link
Collaborator

miraep8 commented Jan 5, 2023

I also think it is a good idea! And I like the phrasing you used along the lines "it is best practice". I think some of the other phrasing might be a bit wordy, but overall I like the change/think it would help new contributors! :)

klam-data pushed a commit to CodeSmithDSMLProjects/sktime that referenced this issue Jan 18, 2023
Adds a separate section in the PR template to ask contributors whether they have added any new tests. 

Fixes sktime#4066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation & tutorials
Projects
None yet
Development

No branches or pull requests

3 participants