Pytest assert rewrite fails when rewriting some parts of cassandra cython code #10844
Comments
Zac-HD
added
type: bug
|
The first step for anyone who wants to work on this will be to get a minimal reproducing example, which we can investigate more easily and later e.g. add to our own test suite. |
|
Sure. That one is easy (requires pyenv + pyenv-virtualenv + installing cassandra-driver): tested on MacOS M1: Here are the minium reproducible steps: pyenv virtualenv 3.9 cassandra-pytest
pyenv activate cassandra-pytest
pip install pytest cassandra-driver
mkdir test
cat >pytest.ini <<EOF
[pytest]
python_files = *.py
EOF
cat >test/test_example.py <<EOF
from cassandra.cluster import Cluster
EOF
pytest testProduces:
When followed by: You get (correctly):
|
|
Also just to capture the installed versions: |
|
@potiuk did you manage to find the problem? I'm hitting the same issue: |
|
Link to problematic code: https://github.com/datastax/python-driver/blob/8c41066330eb04c34eff57153ab2eda810844d5f/cassandra/protocol.py#L699C5-L699C126 # Names match type name in module scope. Most are imported from cassandra.cqltypes (except CUSTOM_TYPE)
type_codes = _cqltypes_by_code = dict((v, globals()[k]) for k, v in type_codes.__dict__.items() if not k.startswith('_'))Didn't verify, but I'm guessing that, in combination with If you can convince datastax/python-driver to add a pytest-specific workaround, I think adding the string Otherwise, pytest is probably not going to change how its assertion rewriting works. My recommendation is not to use |
|
Yeah. I've basically given up and manually add the PYTEST_DONT_REWRITE after installing cassandra in our CI image: Yes I tried to do it already: datastax/python-driver#1142 - where I tried to convince datastax maintainers to add the rewrite comment (which from my point of view would be generally harmless and 0 maintenance - but they decided otherwise, I guess pytest is not going to do anything, so seems like no-one wants to own the problem. Too bad, I am certainly not going to want to die on the hill (especially that it seems that this is a no-ones hill). We have a working workaround in our test env, so i am good. I think using *.py is indeed not default. Though I think it's useful in big projects like Airflow - we had some of our tests not being collected because they were accidentally put in a file without test_ prefix, so we prefer to keep *.py because there is no other way to easily find that you are missing some tests from being collected (and run) - which is quite nasty surprise effect actually, but it's a niche case indeed (especially that pytest will happily run those tests when the file is specified directly. But this is not a big issue in general and if neither Pytest nor Cassandra are concerned - who am I to tell otherwise. It's their choice, some of their user will suffer, and loose time unnecessarily to debug the problem, but well - they are in full rght to make their choice. It's nicely documented with my issues and PRs and people can find workarounds if they look for it, this is one of very legitimate ways of addressing the problem (have it documented how users dealt with it). However I might haveother suggestions for pytest team maybe:
|
|
@potiuk Their problematic code is dict((v, globals()[k]) for k, v in type_codes.__dict__.items() if not k.startswith('_'))They might be more receptive to changes in this line if they are not obviously pytest specific. For example dict((v, globals()[k]) for k, v in type_codes.__dict__.items() if k.endswith('Type'))(should also allow them to remove or dict((v, globals()[k]) for k, v in type_codes.__dict__.items() if not k.startswith(('_', '@'))(kinda pytest specific but hopefully OK).
Hmm I'm not sure off hand why 3rd-party, there's probably something relying on it but might be worth a try to avoid it. |
|
Surely I might be the messenger who sends those messages back-forth - but I have literally 0 context (I understand all the words you have written, butI have no idea about internals of both pytest and cassandra, to assess why and how those changes could have avoided the problem). So I will gently copy your message there and tag you @bluetech if they have more questions maybe that will convince them. |
|
Added comment there: datastax/python-driver#1142 (comment) |
The cassandra hack added in apache#30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file.
|
BTW. I just created a PR with my fixture idea in Airflow apache/airflow#32626 - which also allows us to remove the cassandra hack entirely. And surely, I found a few cases where some of our contributors DID NOT follow the The fixture failing test from badly named files is a very good protection because none of the contributors will be able to run their tests locally if they are placed in a wrongly named file. Without such protection in place, I think So I think |
…32626) The cassandra hack added in #30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file.
…32626) The cassandra hack added in #30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file. GitOrigin-RevId: c6594480e2722513fd082a6c65e30e2504698ba2
…32626) The cassandra hack added in #30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file. GitOrigin-RevId: c6594480e2722513fd082a6c65e30e2504698ba2
…32626) The cassandra hack added in #30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file. GitOrigin-RevId: c6594480e2722513fd082a6c65e30e2504698ba2
…32626) The cassandra hack added in #30315 does not seem to have a chance to get away. Neither Pytest pytest-dev/pytest#10844 nor Datastax datastax/python-driver#1142 want to own the problem for now (though there is a proposal from pytest contributors on how Datastax could refactor their code to avoid the problem) However during the discussion an idea popped in my head on how we could come back to test_* pattern with far less probability of missing some tests that are added to wrong files. Seems that we can add a fixture that will outright fail tests if they are placed if files not following the test_* pattern. While it would not help in case test would be wrongly named in the first place, it would definitely help to not to add more tests in wrongly named files because it will be literally impossible to run the tests added in a wrong file, even if you manualy do `pytest somefile.py` and avoid running collection. I also did a quick check to try to find cases where the test_* file name was already violated and I found (and renamed) two that I have found. It seems it is quite likely that similar mistake could be done in the future - but with the fixture I added it should be far less likely someone adds tests in a wrongly named file. GitOrigin-RevId: c6594480e2722513fd082a6c65e30e2504698ba2
It seems that assert rewrite has a subtle bug that causes crash when collecting and executing code when we are importing cassandra.
Context: When we added
python_files = "*.py"to Apache Airflow in order to not accidentally skip some of our tests ( apache/airflow#30315 ), the PRs started to fail with mysterious:After some (difficult and wild) investigation, it turned out that this is because Pytest assert rewrite fails when trying to rewrite the https://github.com/datastax/python-driver/blob/master/cassandra/type_codes.py file (which apparently comes from Cython integraiton - https://github.com/datastax/python-driver/blob/master/cassandra/type_codes.pxd
This is the most likely reason because either adding
--assert=plainor patching thetype_codes.pyfile withPYTEST_DONT_REWRITEto docsstring solves the problem.I've opened a PR to cassandra to include PYTEST_DONT_REWRITE datastax/python-driver#1142 and in Apache Airflow we have PR to autoamaticallly patch cassandra driver with it apache/airflow#30315, but those are merely workarounds for the problem.
Reproduction:
An easy way to reproduce it:
This results in:
This module contains currently PYTEST_DONT_REWRITE. Remove and save the file.
This results in series of errors for each test being collected:
Mandatory information:
Versions
The output of pip list:
pip listfrom the virtual environment you are usingThe text was updated successfully, but these errors were encountered: