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

Fix 'clean' fixture #59

Merged
merged 4 commits into from
Feb 7, 2022
Merged

Fix 'clean' fixture #59

merged 4 commits into from
Feb 7, 2022

Conversation

kshivakumar
Copy link
Contributor

No description provided.

@laike9m
Copy link
Owner

laike9m commented Feb 4, 2022

Thanks for the fix!

@laike9m
Copy link
Owner

laike9m commented Feb 4, 2022

Currently there are some known test setup issue which make the test suite not able to succeed on GitHub actions..I tried to search for solutions but didn't solve it. Haven't digged too deep though.

Not sure if you're also interested in taking a look
https://github.com/laike9m/pdir2/runs/5062829628?check_suite_focus=true

@kshivakumar
Copy link
Contributor Author

@laike9m

Currently there are some known test setup issue which make the test suite not able to succeed on GitHub actions..I tried to search for solutions but didn't solve it. Haven't digged too deep though.

Not sure if you're also interested in taking a look https://github.com/laike9m/pdir2/runs/5062829628?check_suite_focus=true

I can look into it next week.

@laike9m
Copy link
Owner

laike9m commented Feb 4, 2022

Cool, thank you again

@kshivakumar
Copy link
Contributor Author

kshivakumar commented Feb 4, 2022

@laike9m
I can see that Testing (3.9, ubuntu-latest) failed due to a mypy error which I will try to debug and fix, but I don't understand why other tasks are "cancelled". Is that something I need to look into as well? Or did they get cancelled because one of the tasks failed?

Also, should I raise a separate PR for the mypy error fix?

@laike9m
Copy link
Owner

laike9m commented Feb 5, 2022

I can see that Testing (3.9, ubuntu-latest) failed due to a mypy error which I will try to debug and fix, but I don't understand why other tasks are "cancelled". Is that something I need to look into as well? Or did they get cancelled because one of the tasks failed?

I believe they're cancelled because GitHub Actions saw a failure (3.9)
see https://stackoverflow.com/q/61070925/2142577

I'm fine with disabling fail-fast

@kshivakumar
Copy link
Contributor Author

kshivakumar commented Feb 6, 2022

@laike9m
There were two groups of errors:

  1. Cannot find implementation or library stub for module named "colorama"
  2. Cannot find implementation or library stub for module named "pytest"
    Both had the same root cause - pytest and colorama packages do not have type annotations that mypy looks for by default.

There are two ways to fix this

  • Install stub packages to pytest and colorama(types-colorama)
  • Suppress the errors

I decided to suppress the errors because:

  1. colorama is used only at once place (to handle Windows platform in api.py)
  2. pytest is a dev dependency

I originally intended to suppress the errors by adding markers inside any of the config files - tox.ini, pyproject.toml, mypy.ini. But no matter which config file I tried, I was not able to make it work.

UPDATE: I was finally able to make it work, see the latest commit

mypy looks for configuration in .mypy.ini and pyproject.toml files whereas pytest-mypy checks in mypy.ini and conftest.py. This creates confusion where exactly the mypy markers are supposed to go, which file overrides which.

On top of that, initially I was not able to replicate 'Cannot find implementation or library stub for module named "pytest"' in my local when using virtualenv. Only when I used pdm and ran the commands in the same order as Github Actions I could replicate the error.

I finally decided to add type: ignore at places where we are importing colorama and pytest. This is definitely not the best approach because every time a new test file containing import pytest is added, the marker type: ignore needs to be added. In case of colorama it's acceptable since we are importing it only once.

@kshivakumar
Copy link
Contributor Author

kshivakumar commented Feb 6, 2022

I'm fine with disabling fail-fast

Never mind, the existing behaviour actually makes sense.

@kshivakumar kshivakumar force-pushed the tc_ffix branch 2 times, most recently from 7b1b0e0 to d484ff9 Compare February 6, 2022 09:53
@laike9m
Copy link
Owner

laike9m commented Feb 7, 2022

Ignoring the error makes sense to me. Thank you again!

@laike9m laike9m merged commit e138a10 into laike9m:master Feb 7, 2022
@kshivakumar kshivakumar deleted the tc_ffix branch October 14, 2023 05:53
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

Successfully merging this pull request may close these issues.

2 participants