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

Correct tests handling paths to files where paths to directories are expected #8947

Merged

Conversation

Cypher1
Copy link
Contributor

@Cypher1 Cypher1 commented Feb 5, 2024

I think these tests are testing slightly incorrect behaviour. Fixing them should unblock another change I'd like to suggest but we should also ensure we aren't testing behaviours that aren't intended.

@Cypher1 Cypher1 changed the title Correct tests for handling of paths to files where paths to directories are expected Correct tests handling paths to files where paths to directories are expected Feb 5, 2024
@radoering
Copy link
Member

Good catch. Did you notice that there is a third test that failed in https://github.com/python-poetry/poetry-core/actions/runs/7782092252/job/21217798889?pr=695? It looks like with_empty_repositories_key does not exist at all. We probably can just use another fixture without repositories.

@Cypher1
Copy link
Contributor Author

Cypher1 commented Feb 5, 2024

Ah! Thanks for the catch. I went to bed before I could see the results.

@Cypher1 Cypher1 force-pushed the fixForTestDirectoryPathsBeingTomls branch from aeb2f03 to 192d497 Compare February 5, 2024 20:29
@Cypher1
Copy link
Contributor Author

Cypher1 commented Feb 5, 2024

I thought it wise to add that missing fixture, could you check that I've understood the test's intent?

In particular: I used the local_config fixture as a starting point, so I might have missed something that should be removed from that.

@Cypher1
Copy link
Contributor Author

Cypher1 commented Feb 5, 2024

@radoering I'm wondering if that test actually needs to exist. The empty repository key that it checks is enforced by the schema validation in Factory.

I've updated it to check that schema validation is performed, but this may be redundant or the behaviour may be wrong (as the previously tested behaviour may be desired). Or perhaps the 'empty_repository_key' actually meant an unset repository key and I've gone in the wrong direction.

Thanks in advance

@radoering
Copy link
Member

The test was introduced in #7910 so the goal was to increase test coverage. If I remove the test, the line coverage for config.py decreases from 95% to 93%, so I think it makes sense to have such a test. The test is not about repository but about repositories and especially just tests the command if there are no repositories. (That's why the tests works if some abitrary sample project without repositories is chosen.) Thus, you took the wrong turn.

@Cypher1 Cypher1 force-pushed the fixForTestDirectoryPathsBeingTomls branch from dd7f38a to 7ed5424 Compare February 7, 2024 07:35
@Cypher1 Cypher1 force-pushed the fixForTestDirectoryPathsBeingTomls branch from 7ed5424 to 4926932 Compare February 7, 2024 07:45
@radoering radoering merged commit 3e98fd8 into python-poetry:master Feb 7, 2024
20 checks passed
@Cypher1 Cypher1 deleted the fixForTestDirectoryPathsBeingTomls branch February 7, 2024 20:01
Copy link

github-actions bot commented Mar 9, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants