laike9m / pdir2

Pretty dir() printing with joy:beer:
MIT License
1.33k stars 47 forks source link

Fix 'clean' fixture #59

Closed kshivakumar closed 2 years ago

laike9m commented 2 years ago

Thanks for the fix!

laike9m commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Cool, thank you again

kshivakumar commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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

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 commented 2 years ago

I'm fine with disabling fail-fast

Never mind, the existing behaviour actually makes sense.

laike9m commented 2 years ago

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