Skip to content

Conversation

@robvdl
Copy link
Contributor

@robvdl robvdl commented Mar 11, 2024

The tests directory shouldn't have an __init.py__ and the imports into your package should just use the package name not src.packagename.

See: https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#tests-outside-application-code

I've ran the tests locally in a virtualenv to verify it works, and the tests passed locally.

The tests directory shouldn't have an init.py and the imports into your package should just use the package name not "src.packagename".

See: https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#tests-outside-application-code
@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

The extra blank lines is so that it conforms to isort.

"time" is standard library and belongs in the first group

"pytest" is third party so belongs in the next group

"thread" is your code so belongs in the next group

I normally wouldn't do multiple things in one commit, but I was already touching those lines anyway.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

OK so the tests are passing for me locally but not in CI/CD so you have something off with your CI setup.

The import "thread" should work. You shouldn't have to import it through src, if you are doing that, it's wrong.

image

Need to inspect the CI setup next, that is where the problem will lie.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

Something off with your poetry setup I noticed.

If I do poetry install on your module it installs nothing, I do a pip freeze and all I see is a blank virtualenv, no thread package is installed.

If I do pip install on your module it installs fine, I do a pip freeze, I see the thread package installed

Your CI setup is doing poetry install and that is why it's not installing the module thread somehow and that is why the tests can't find that module, so you've resorted to doing the src.thread thing which isn't the answer off course.

@caffeine-addictt
Copy link
Member

CI tests are ran from the CLI

python -m pytest -sv

I don't believe this behavior would differ. At least for me, its also failing locally.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

If I install it like this it works by the way:

pip install .

But that won't install the dev dependencies, and pip install .[dev] doesn't work.

you could install the dev dependencies manually then run pip install ., but I don't understand the whole dance in using poetry to export a requirements.txt then install from that requirements.txt

checkout github.com/wainuiomata/sambal, pyproject.toml that is not using poetry

but I do have some other projects that do use poetry, just not open source

because module "thread" isn't installing somehow
@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

See what I mean, that second commit just runs "pip install ." and it magically starts working.

This is what I mean something seems off with the poetry install, I run "poetry install" on my project and it installs the module. I run "poetry install" on your project and it doesn't.

We could compare pyproject.toml I guess, but running "pip install ." is a quick fix.

@caffeine-addictt
Copy link
Member

This is quite odd behavior. poetry install does install thread locally for me but doesn't seem to in CI. That why dependencies were installed through the generated requirements.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I am looking at my only poetry based projects, but they haven't got a src-based layout yet. Other than that they are quite simple.

[tool.poetry]
name = "packagename"
version = "1.0.0"
description = ""
authors = []
license = ""
readme = "README.md"

[tool.poetry.dependencies]
python = "^3.8"

[tool.poetry.dev-dependencies]

It seems your dev dependencies have .group in it, mine doesn't

I haven't done any of this with mine:

packages = [
  { include = "thread", from = "src" },
  { include = "thread/py.typed", from = "src" },
]
include = [{ path = "tests", format = "sdist" }]

I was thinking, perhaps try regenerating the poetry lockfile???

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

Also maybe the poetry version, I was using poetry 1.5.1

@caffeine-addictt
Copy link
Member

Ah, that may be it. I'm running poetry 1.7.1 here and poetry lock only updates the ruff dependency.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I'm updating my poetry now, I had it installed globally

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I upgraded poetry and still no luck. It installs, but it doesn't install anything.

image

(that was in a fresh virtualenv, it installed nothing at all)

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

pip freeze should list the packages that installed but it isn't, I'll try deleting and regenerating the lockfile

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

deleting and regenerating the lockfile doesn't fix it sadly

@caffeine-addictt
Copy link
Member

Thats odd
image

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

to be honest I don't think you need this at all

packages = [
  { include = "thread", from = "src" },
  { include = "thread/py.typed", from = "src" },
]
include = [{ path = "tests", format = "sdist" }]

it's supposed to auto detect a src-based layout and I think having that init.py in tests earlier might have been why this was needed

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I'm just reading this now https://browniebroke.com/blog/convert-existing-poetry-to-src-layout/

I think that poetry needs extra work for a src based layout, compared to how I did it in github.com/wainuiomata/sambal which doesn't use poetry, it auto-sensed src based layout but I think Poetry needs a little guidance.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I'm starting to think Poetry hates src-based layout but I'm not sure that is it.

@caffeine-addictt
Copy link
Member

caffeine-addictt commented Mar 11, 2024

I'm not too sure if removing py.typed explicitly will place it in dist and be sufficient.

But referencing from poetry's repository, doesn't look like there is a need for it... We could remove it and see how it goes in v1.0.1.

Only the include for tests seem necessary. https://github.com/python-poetry/poetry/blob/main/pyproject.toml#L17

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I am stumped, I've already tried all those things and it had no effect, so I've reversed them again anyway.

I have never seen such a thing in my life. I also tried moving it up out of the src directory, but it had no effect.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

What is CITATION.cff it has a typo reposistory-artifact: https://pypi.org/project/thread

reposistory, not sure if that would do it

@caffeine-addictt
Copy link
Member

caffeine-addictt commented Mar 11, 2024

CITATION.cff is for citing the repository

image

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I see why it isn't installing anything, is because (if you run it with -vvv) for very verbose you'll notice it won't bother installing typing-extensions because it doens't need it:

image

At least on Python 3.10 here it doesn't need typing-extensions so it won't install anything.

As for the module itself I think it installs it in editable mode.

I think all you really need is this:

[tool.pytest.ini_options]
pythonpath = ["src"]

and Eurka it works

from here: https://browniebroke.com/blog/convert-existing-poetry-to-src-layout/

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

But when I do pip freeze there is still nothing in the virtualenv, so I really don't know what is going on unless something has changed in Poetry.

It seems that Poetry is not using the virtualenv I've given it anymore, but it's using it's own in /home/rob/.cache/pypoetry and it's doing my head in.

image

So it says it installed all those things but not in the current virtualenv, why? I've always used Poetry this way.

@caffeine-addictt
Copy link
Member

caffeine-addictt commented Mar 11, 2024

It's using to proper virturalenv for me though

image

This is becoming infuriatingly confusing haha

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

Yeah that is the problem, Poetry has taken on managing the virtualenv now and it's getting in the way.

I hadn't come across this yet myself as I generally run stuff in Docker.

But this is a bit dissapointing because I just want Poetry to install into the current virtualenv but it is trying some weird cached one:

(thread) rob@rob-pc:~/Projects/thread$ poetry env info

Virtualenv
Python:         3.10.12
Implementation: CPython
Path:           /home/rob/.cache/pypoetry/virtualenvs/thread-zV_EQ1l7-py3.10
Executable:     /home/rob/.cache/pypoetry/virtualenvs/thread-zV_EQ1l7-py3.10/bin/python
Valid:          True

Base
Platform:   linux
OS:         posix
Python:     3.10.12
Path:       /usr
Executable: /usr/bin/python3.10

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

Check this out then, I delete the env and it falls back to the right one (/home/rob/.virtualenvs/thread)

(thread) rob@rob-pc:~/Projects/thread$ poetry env remove --all
Deleted virtualenv: /home/rob/.cache/pypoetry/virtualenvs/thread-zV_EQ1l7-py3.10
(thread) rob@rob-pc:~/Projects/thread$ poetry env info

Virtualenv
Python:         3.10.12
Implementation: CPython
Path:           /home/rob/.virtualenvs/thread
Executable:     /home/rob/.virtualenvs/thread/bin/python
Valid:          True

Base
Platform:   linux
OS:         posix
Python:     3.10.12
Path:       /usr
Executable: /usr/bin/python3.10

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

and now it works (after running poetry env remove --all)

image

But remember I also upgraded Poetry in between

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

So poetry was trying to manage it's own env rather than installing into the env I gave it and that meant it only "looked" like nothing was installing.

@caffeine-addictt
Copy link
Member

Seems like quite the troublesome feature of poetry

@caffeine-addictt
Copy link
Member

I guess for the time being, tests can keep using pip install . for the time being unless you've already figured it out

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I'm not sure where to go from here. I'll leave this PR open but it feels running pip install . in CI was a hack.

I'm not sure if running poetry env remove --all in CI is such a great thing either, feels equally as much of a hack to me.

But you may want to look at that cherry picking typo in CITATION.cff if you want at least.

@caffeine-addictt
Copy link
Member

caffeine-addictt commented Mar 11, 2024

Yea agreed. I'll co-author you in another commit.

caffeine-addictt added a commit that referenced this pull request Mar 11, 2024
Related #59

Co-authored-by: Rob van der Linde <robvdl@gmail.com>
@caffeine-addictt caffeine-addictt mentioned this pull request Mar 11, 2024
12 tasks
@caffeine-addictt caffeine-addictt merged commit 89c6244 into python-thread:main Mar 11, 2024
@caffeine-addictt
Copy link
Member

caffeine-addictt commented Mar 11, 2024

Oh my god, I merged the wrong PR 🤣

@caffeine-addictt
Copy link
Member

You can create another PR and reference this in the future. Apologies for the inconvenience 🥲

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

Well it's not that bad, that branch was working.

@robvdl
Copy link
Contributor Author

robvdl commented Mar 11, 2024

I haven't got CI going on my own project yet beyond linting ruff

because I need to compile Samba master from source as one of my dependencies which makes CI setup too much trouble for me right now so I run tests locally for the time being until Samba 4.21 gets released.

@robvdl robvdl deleted the fix-tests branch March 11, 2024 08:40
caffeine-addictt added a commit that referenced this pull request Apr 28, 2024
Reverting the revert of #59

Ref: #59
Ref: #61

Co-authored-by: Rob van der Linde <robvdl@gmail.com>
caffeine-addictt added a commit that referenced this pull request Apr 28, 2024
Might resolve the previous unresolved ci issue in #59
@caffeine-addictt caffeine-addictt mentioned this pull request Apr 28, 2024
14 tasks
caffeine-addictt added a commit that referenced this pull request Apr 28, 2024
Update tests

Unreverts #59 
Reverts #61


Co-authored-by: Rob van der Linde <robvdl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants