chore: Move tests to tests/ dir

[nealrichardson]

This is just the move. I'll follow up with the mock response refactoring after, just wanted to get this out quickly since it will conflict with everything.

Closes #58

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
282	246	87%	80%	🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 1a2e5f9 by action🐍

[nealrichardson]

FYI coverage is "down" because previously it was counting the test files as lines of package code to be covered by tests (miraculously, they were all 100% covered!)

[tdstein]

I prefer keeping unit tests "as part of application code." I've become accustomed to side-by-side src and test files after working in JavaScript and Go eco-systems for some time.

I used to be adamant about keeping tests "outside application code," but I switched after dealing with the nuances of using editable installs.

Double check me, but It looks like this setup has a similar issue. If I run the following, the tests run against the installed v0.0.dev0 build instead of the local source code.

pip install posit-sdk==0.0.dev0
make tests

You can verify this by placing an assert False in src/posit/__init__.py

This will then fail, while the previous does not.

pip install -e .
make tests

This isn't a deal breaker, but it's annoying to troubleshoot if you don't know how installs work.

[tdstein]

Need to keep - if: always() here. Otherwise, coverage won't report if make cov-xml fails.

[nealrichardson]

I don't think you want if: always because it's only running on one branch of the matrix. And anyway, if make cov-xml fails, how can coverage report?

[tdstein]

Actually, should it be...?

- if: ${{ !cancelled() && matrix.python-version == '3.12' }}

https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions

make cov-xml will fail if the coverage is below report requirements but will still report the coverage. We want this step to run regardless so that the coverage report is added to the Pull Request.

Here is an example of the behavior: #67

[nealrichardson]

Ah, got it. Thanks for the demonstration.

Would it be better to just remove fail_under = 80 from .coveragerc? I'm not sure it's helping us more than having the coverage report on the PR.

[tdstein]

Either works for me.

[tdstein]

Have you verified that this gives you the functionality you want?

https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html?highlight=import%20mode#import-modes

One drawback however is that test modules are non-importable by each other. Also, utility modules in the tests directories are not automatically importable because the tests directory is no longer added to sys.path.

If I understand this correctly, this means that abstracting mocked classes into their own module won't work.

[nealrichardson]

I have not verified. I was just blindly following the pytest docs recommendation: "For new projects, we recommend to use importlib import mode." Happy to remove if/when it's a problem.

[tdstein]

@nealrichardson - thoughts on this as an alternative? #62

Going with "option 1" below.

[nealrichardson]

@nealrichardson - thoughts on this as an alternative? #62

Following https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#choosing-a-test-layout-import-rules, I went with option 1. Looks like #62 is option 2.

• Option 1: "Putting tests into an extra directory outside your actual application code might be useful if you have many functional tests or for other reasons want to keep tests separate from actual application code (often a good idea)"
• Option 2: "Inlining test directories into your application package is useful if you have direct relation between tests and application modules and want to distribute them along with your application"

Based on that, I think Option 1 is a better fit for us. We're going to be building up more functional tests and setup and stuff that is going to make having the tests inside the submodules unwieldy. Plus, for better or worse, Option 1 seems to be more common, and that comes with other benefits--the fact that the coverage tool was counting test files as application files to check for coverage is a sign of that.

I think we solve the editable-installs issue by making clear guidance on how to develop on the project. I don't think it will be a big deal. And the fact that I can run the current test suite against an installed version of the package sounds like a feature.

[tdstein]

I think we solve the editable-installs issue by making clear guidance on how to develop on the project.

To this point, should we...

• modify Makefile to run pip install -e . as part of make deps?
• change make install to install the wheel from dist instead of performing an editable install?

[nealrichardson]

I think we solve the editable-installs issue by making clear guidance on how to develop on the project.

To this point, should we...

• modify Makefile to run pip install -e . as part of make deps?

IDK, that doesn't sound right to me.

• change make install to install the wheel from dist instead of performing an editable install?

Why would I want to do that as a developer?

To me, it doesn't seem too bad to say that to get started, create a virtualenv and then make deps && make install. Then develop as you like. The main make steps are all single lines, which is probably why a lot of projects don't bother with Makefiles.

[tdstein]

In my experience, the convention is for make to produce the distributable artifact and for make install to take that artifact and install it on the system. But, this is getting out of the scope of your original objective. I'll carry it to a separate issue.

[nealrichardson]

In my experience, the convention is for make to produce the distributable artifact and for make install to take that artifact and install it on the system. But, this is getting out of the scope of your original objective. I'll carry it to a separate issue.

Yeah that tracks with my experience in C/C++ too but since Python is not a compiled language, I think the expectations are different (if there are expectations at all).