saghul / pycares

Python interface for c-ares
https://pypi.org/project/pycares/
MIT License
162 stars 74 forks source link

Cleanup tests #177

Closed mcepl closed 1 year ago

saghul commented 1 year ago

Somehow they all fail..

mcepl commented 1 year ago

Right, you don’t care about other peoples’ environments, why should I care about yours? Yes, I have reproduced all those errors you have closed without checking.

saghul commented 1 year ago

What's with the hostility? Your PR says you cleaned up the tests, and they all fails because of your PR, look at the output:

Run python -munittest -v tests.tests
Traceback (most recent call last):
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/__main__.py", line 18, in <module>
    main(module=None)
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/main.py", line 100, in __init__
    self.parseArgs(argv)
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/main.py", line 147, in parseArgs
    self.createTests()
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/main.py", line 159, in createTests
    self.module)
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/loader.py", line 220, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/loader.py", line 220, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/Users/runner/hostedtoolcache/Python/3.7.15/x64/lib/python3.7/unittest/loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
  File "/Users/runner/work/pycares/pycares/tests/__init__.py", line 1
    This file intentionally left blank.

Do you think it's reasonable of me to take a PR that literally breaks the CI in all combinations?

If there is a way to make the tests work in different setups, sure, let's have it! But then the CI also needs updating to it continues to function.

mcepl commented 1 year ago

What's with the hostility? Your PR says you cleaned up the tests, and they all fails because of your PR, look at the output:

Sorry, uncommented comment in __init__.py is a bug. I am sorry about wasting your time with it. My patch in our build system is now just:

---
 tests/__init__.py |    1 +
 1 file changed, 1 insertion(+)

--- /dev/null
+++ b/tests/__init__.py
@@ -0,0 +1 @@
+# This file intentionally left blank.

Hostility? Well, I would call it a realistic assessment of your attitude after reading comments (and closed tickets) like https://github.com/saghul/pycares/issues/145#issuecomment-839753560, https://github.com/saghul/pycares/issues/163#issuecomment-945539915 and others. If you don’t care about your users reports, I don’t see the reason why I should care about your tests.

saghul commented 1 year ago

Sorry, uncommented comment in __init__.py is a bug. I am sorry about wasting your time with it. My patch in our build system is now just:

Would you like to reopen the PR and test it out?

Hostility? Well, I would call it a realistic assessment of your attitude after reading comments (and closed tickets) like #145 (comment), #163 (comment) and others. If you don’t care about your users reports, I don’t see the reason why I should care about your tests.

That is your interpretation, you can ask but ultimately you were not involved in any of the issues so you are just assuming.

Reality is the current test suite uses real DNS and that comes with some drawbacks, like some tests may fail depending on configuration of the machine were they run on. Thus, the CI is the only common way to validate the library.

If you have any suggestions I'm happy to hear them, but so far I have no better option.

mcepl commented 1 year ago

Would you like to reopen the PR and test it out?

Really, the diff above is the whole patch in its entirety. If you care, than I forfeit any rights in it and put it into public domain.

Reality is the current test suite uses real DNS and that comes with some drawbacks, like some tests may fail depending on configuration of the machine were they run on. Thus, the CI is the only common way to validate the library.

Ah, so it is not mocked? Than the problem is obvious … build environments of all Linux (and non-Linux as well) distributions are isolated from the network, so if you require network access for your tests, they will fail.