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

Policy Suggestion: Require Mechanics Fixing PRs to Have Unit Tests #8868

Open
gigalh128 opened this issue Aug 16, 2022 · 3 comments
Open

Policy Suggestion: Require Mechanics Fixing PRs to Have Unit Tests #8868

gigalh128 opened this issue Aug 16, 2022 · 3 comments
Assignees

Comments

@gigalh128
Copy link
Contributor

gigalh128 commented Aug 16, 2022

Showdown has issues with buggy move mechanics being found in production. Ideally, we would have tests for moves to catch these before this happens. I know we can't feasibly write comprehensive tests to prevent all of these now, as there are just too many moves. However, we can mitigate this, and one simple idea is to require anyone who makes a PR fixing a certain mechanic to also make unit tests that demonstrate the mechanic has been fixed correctly. This is what I have done in my gen 1 fix for haze and disable. By implementing this policy, we can build a better culture of writing unit tests so that bugs are found before reaching production, and doing so will also get people more familiar with the Showdown codebase.

Let me know your thoughts on this policy suggestion.

@gigalh128 gigalh128 changed the title Policy Suggestion: Mechanics Fixing PRs Must Have Tests Policy Suggestion: Require Mechanics Fixing PRs to Have Unit Tests Aug 16, 2022
@AnnikaCodes
Copy link
Collaborator

I support this. Unit tests are generally helpful and mechanics are relatively easy to test. I do think that for new contributors, we should make it clear that maintainers will help them to write unit tests (testing frameworks can be hard to wrap your head around as a new contributor).

@DaWoblefet
Copy link
Member

DaWoblefet commented Sep 13, 2022

I would prefer this as an encouraged suggestion, not mandatory. Certain small mechanics fixes don't warrant tests, and I wouldn't want someone to say, open a PR with "Prevent Dynamax Cannon from being called by Instruct" with a test where it's not really needed. I am in agreement with Annika that seasoned maintainers helping with tests, or straight up providing tests, is a great middle ground if it's a new developer.

@Zarel
Copy link
Member

Zarel commented Apr 30, 2023

The reason I've never required this is because it's kind of hostile to new contributors. "Thanks for helping us for free, now here's some more work for you."

I've also been on the other end of this ( mochajs/mocha#2746 ), where I submit a bugfix, and they demand specific kinds of tests that aren't possible, and the bug sits unfixed because that's the rules.

I agree with Wob; encouraging more unit tests is good, but I think it would be bad to make it a requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants
@Zarel @DaWoblefet @AnnikaCodes @gigalh128 and others