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

Contributor docs: there should be guidance for writing tests #24308

Open
oscarbenjamin opened this issue Nov 24, 2022 · 2 comments
Open

Contributor docs: there should be guidance for writing tests #24308

oscarbenjamin opened this issue Nov 24, 2022 · 2 comments

Comments

@oscarbenjamin
Copy link
Contributor

I've just reviewed a number of recent open PRs and a common theme is new contributors not understanding what tests are or how to write them.

The contributing section of the docs mentions very briefly how to run the tests:

https://docs.sympy.org/latest/contributing/index.html#contributing
https://docs.sympy.org/latest/contributing/dev-setup.html#run-the-tests

That's a very short section though and could be expanded a lot to explain how to do useful things with pytest using e.g. -nauto and --lf which are extremely useful options. It also does not explain how to run the doctests.

The old Wiki page for development has more information about running the tests and also mentions the idea of writing tests but doesn't really explain how to do it or show any examples:
https://github.com/sympy/sympy/wiki/Development-workflow

There is no information anywhere about writing tests. Since this is a basic requirement for pretty much every PR it should be explained somewhere. In particular new contributors need to know the following things:

  1. That pretty much every PR should have tests.
  2. What the purpose of tests are in general.
  3. How to run the tests locally and how to also interpret the CI output when tests fail.
  4. Where to find the test code to look at it and also where to add more tests.
  5. What sort of things should be tested.
  6. How to write the tests and how to use things like raises etc.
  7. That sometimes it is necessary to review or change the existing tests.
@oscarbenjamin
Copy link
Contributor Author

Some points to add to that list:

  1. It is generally necessary for all the tests to pass before a PR can be merged.
  2. Sometimes the tests in CI will fail for unrelated reasons (usually an update of another library).

@asmeurer
Copy link
Member

I'll look into this for #23879

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

No branches or pull requests

2 participants