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

Proposal: Move our unit tests from tests/ to src/. #7489

Closed
benjyw opened this issue Apr 3, 2019 · 19 comments
Closed

Proposal: Move our unit tests from tests/ to src/. #7489

benjyw opened this issue Apr 3, 2019 · 19 comments
Assignees

Comments

@benjyw
Copy link
Contributor

benjyw commented Apr 3, 2019

As best I can tell, having a separate tests tree is a legacy of simplistic build tools such as Maven, that expect to bundle entire source trees and don't want to bundle tests along with the code.

Pants, however, does not suffer from this limitation, so we seem to be adhering to that style unnecessarily.

There are strong advantages to having unit tests (and maybe even all tests) live alongside the code they test:

  • It's easier to find the corresponding tests when reading the code.
  • It subtly encourages people to write more tests, as it's easy to see at a glance which source files are missing them.
  • It showcases the fact that Pants supports this.

We've been doing this at Toolchain and it works really well.

I propose that we:

  1. Require new unit tests to be sibling files of the code they test, with the suffix _test, so the tests for foo.py go in foo_test.py. Ordered directory listings then make it very easy to see which files have tests.

  2. After a few weeks of seeing how we like it, consider requiring all new tests (including integration tests) to be in the src/ tree (with the suffix _integration_test.py).

  3. Consider moving all existing unit tests (mechanically, of course) to src/.

Note that the default sources for python_library are *.py - (test_*.py, *_test.py), and the default sources for python_tests are (test_*.py, *_test.py), so this pattern is supported by these defaults.

Please comment with your opinions!

@jsirois
Copy link
Contributor

jsirois commented Apr 3, 2019

Sounds good to me with the addition of Test.java, Test.scala, Spec.scala suffixes for the java (80 existing files) and scala (4 existing files) cases maybe after we complete phase 3 above for python.

@stuhood
Copy link
Member

stuhood commented Apr 3, 2019

I'm willing to give this a try.

I love default sources, and strongly encourage their use. But I don't think they'll be much help here, because we have so many huge directories/modules. Having said that, that portion just seems like a continuation of the status quo (currently two directories containing lots of targets each, soon 1 directory containing lots of targets).

@benjyw
Copy link
Contributor Author

benjyw commented Apr 3, 2019

Yeah, although I would like us to move towards one target per directory, and split directories where it makes sense to do so. But that's almost orthogonal to this.

@benjyw
Copy link
Contributor Author

benjyw commented Apr 3, 2019

@jsirois The java/scala cases are already taken care of in default sources.

@jsirois
Copy link
Contributor

jsirois commented Apr 3, 2019

Ack - I was just referring to an edit of your proposal. It talks about src/ but assumes src/python/

@cosmicexplorer
Copy link
Contributor

I strongly appreciate the idea of colocating all tests with their sources, including integration tests. Rust happens to go one further and keep unit tests in the same file -- I don't think we would go that far, but having examples of using our python source code in the same directory as the source seems like a very good thing for potential and current contributors. This also avoids having directory trees in tests/ bitrot and start to diverge from src/ over time, which makes it difficult to know where to put new tests or find existing ones.

@benjyw
Copy link
Contributor Author

benjyw commented Apr 3, 2019

Gotcha, yes.

@illicitonion
Copy link
Contributor

My only hesitation here is that right now I have IntelliJ set up to treat source directories and test directories differently (e.g. it's easy to filter out test-only search results), and I would be sad to lose that, but as this layout is pretty common in the real world I assume IntelliJ has some fix here (though I couldn't find it at a quick glance).

@stuhood
Copy link
Member

stuhood commented Apr 4, 2019

My only hesitation here is that right now I have IntelliJ set up to treat source directories and test directories differently (e.g. it's easy to filter out test-only search results), and I would be sad to lose that, but as this layout is pretty common in the real world I assume IntelliJ has some fix here (though I couldn't find it at a quick glance).

Mmm, yea, that is annoying. cc @wisechengyi

My understanding is that in IDEA, modules always own an entire directory. So in cases where 1:1:1 is not followed (...and this suggestion would decrease those), it gloms multiple targets together, which can create IDEA module cycles.

@jsirois
Copy link
Contributor

jsirois commented Apr 4, 2019

Well, this is a very specific style of 1:1:1 breaking. If test targets never depend on each other and only on prod and prod never depends on test, I think you never get a cycle from this.

@benjyw benjyw self-assigned this Apr 5, 2019
@benjyw
Copy link
Contributor Author

benjyw commented Apr 5, 2019

@illicitonion Are you referring to JVM code? I use IntelliJ with the Python plugin (which is basically PyCharm under the IntelliJ skin) and AFAICT it doesn't have a "Tests" designation for Python source code directories (it does for JVM code though). I mostly care about our Python code right now, I don't mind punting on our JVM code.

@illicitonion
Copy link
Contributor

illicitonion commented Apr 5, 2019

@benjyw You can mark Python directories as a test sources directory. e.g. in this screenshot of the find dialog, the green backgrounded things are test things (or test infra), and the white backgrounded things are production things.

Find usages also groups by Production and Test, as per the other screenshot.

Screenshot 2019-04-05 at 15 23 12

Screenshot 2019-04-05 at 15 25 28

@benjyw
Copy link
Contributor Author

benjyw commented Apr 5, 2019

Oh yes, I see now, you can designate test roots, but not individual test dirs.

benjyw added a commit that referenced this issue Apr 10, 2019
This will allow us to start moving tests from the tests folder to the src folder. See #7489.
@jsirois
Copy link
Contributor

jsirois commented Apr 20, 2019

@illicitonion perhaps scopes are the right tool here?:
image

I never used them before, but just defined 'Python Tests' and the 'Python Code' (as the negation of 'Python Tests'). These scopes can be used in the find dialogs to, for example, filter out tests using the 'Python Code' scope.

@wisechengyi
Copy link
Contributor

wisechengyi commented Apr 20, 2019 via email

@benjyw
Copy link
Contributor Author

benjyw commented Apr 20, 2019

Sorry late to the party. One needs to mark a directory as test sources. If the structure looks like src/test/Lang, then this needs to happen # of Lang times.

The idea is that there would be no test directories. Tests would be siblings of the code they test.

@illicitonion
Copy link
Contributor

@illicitonion perhaps scopes are the right tool here?:
image

I never used them before, but just defined 'Python Tests' and the 'Python Code' (as the negation of 'Python Tests'). These scopes can be used in the find dialogs to, for example, filter out tests using the 'Python Code' scope.

Scopes look indeed like they can do what I want - I had no idea they existed. Thanks!

@benjyw
Copy link
Contributor Author

benjyw commented Apr 23, 2019

Oooh, that is a nifty feature!

benjyw added a commit to benjyw/pants that referenced this issue Oct 31, 2019
So that future tests under src/ can depend on them.

See pantsbuild#7489
Eric-Arellano added a commit that referenced this issue Nov 6, 2019
…nts/testutil` (#8400)

### Problem
We are in the process of moving our tests to live within the `src` directory, rather than `tests` (#7489). We should consequently have our core utility code live in `src`.

Further, the current utils are spread out across several folders, which makes discovery more difficult.

### Solution
Create a new `pants/testutil` namespace with all of the files used by the `pants.testinfra` wheel. Specifically, it has this directory structure:

```
src/python/pants/testutil
├── BUILD
├── __init__.py
├── base
│   ├── BUILD
│   ├── __init__.py
│   └── context_utils.py
├── console_rule_test_base.py
├── engine
│   ├── BUILD
│   ├── __init__.py
│   ├── base_engine_test.py
│   └── util.py
├── file_test_util.py
├── git_util.py
├── interpreter_selection_utils.py
├── jvm
│   ├── BUILD
│   ├── __init__.py
│   ├── jar_task_test_base.py
│   ├── jvm_task_test_base.py
│   ├── jvm_tool_task_test_base.py
│   └── nailgun_task_test_base.py
├── mock_logger.py
├── option
│   ├── BUILD
│   ├── __init__.py
│   └── fakes.py
├── pants_run_integration_test.py
├── pexrc_util.py
├── process_test_util.py
├── subsystem
│   ├── BUILD
│   ├── __init__.py
│   └── util.py
├── task_test_base.py
└── test_base.py
```

We can't yet delete the equivalent files in `pants_test` due to a public API, so we instead have those files simply re-export the implementation in `pants/testutils`.

#### Does not move some util code
There are several util files not exposed to the `pantsbuild.testinfra` wheel, like `engine/scheduler_test_base.py`. To reduce the scope of this PR, we keep those there for now. Presumably, they will be able to be moved into `pants/testinfra` without a deprecation cycle.

#### Creates new wheel `pantsbuild.pants.testutil`
We had to create a new package, rather than using `pantsbuild.pants.testinfra`, due to rules about ownership of files (#8400 (comment)).

In a followup, we will deprecate `pantsbuild.pants.testinfra`.

### Result
We can now either use `pants.testutil` or the conventional `pants_test` imports. Both have the same functionality.

In a followup PR, we will deprecate the `pants_test` version.
Eric-Arellano added a commit that referenced this issue Nov 20, 2019
This allows for us to quickly see in ordered directly listings all the tests for sibling implementations, e.g. `specs.py` and `specs_test.py`.

This was the original intent of #7489.

Notably, we must name ITs `foo_integration_test.py`, not `foo_test_integration.py`, for Pytest to discover the target and for our `python_tests` target type to properly glob the file. This contrasts with the proposal in #7489.
@Eric-Arellano
Copy link
Contributor

This has mostly been done for V2 code. V1 code is labeled as wont-fix (unless anyone wants to spend the time on it).

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

7 participants