Bitmessage/PyBitmessage-2024-08-21
Archived
2
0
Fork 0
Code Issues 168 Pull Requests 53 Actions Packages Projects Releases 5 Wiki Activity

Fix broken tests #1683

Closed
PeterSurda wants to merge 1 commits from teardown-test1 into v0.6
pull from: teardown-test1
merge into: Bitmessage:v0.6
Conversation 29 Commits 1 Files Changed 9 +190 -43
PeterSurda commented 2020-10-23 13:19:22 +02:00 (Migrated from github.com)
Copy Link
  • setting BITMESSAGE_HOME doesn't always work, so we wrote a function to do this
  • logger and BMConfigParser now clear old config when loading new
  • add method for flushing log files
  • rework logic in test_logger
  • detect thread names on UNIX-like OSes
  • be more accurate when starting or stopping a process, better waiting mechanisms
  • add extra cleanup during TestProcess start
- setting `BITMESSAGE_HOME` doesn't always work, so we wrote a function to do this - `logger` and `BMConfigParser` now clear old config when loading new - add method for flushing log files - rework logic in `test_logger` - detect thread names on UNIX-like OSes - be more accurate when starting or stopping a process, better waiting mechanisms - add extra cleanup during `TestProcess` start
😕 1
g1itch commented 2020-10-23 15:51:25 +02:00 (Migrated from github.com)
Copy Link

Ooogh, it looks sick ):

Ooogh, it looks sick ):
g1itch (Migrated from github.com) reviewed 2020-10-23 15:55:22 +02:00
.travis.yml
g1itch (Migrated from github.com) commented 2020-10-23 15:55:21 +02:00

Sad. I didn't planned to bound the tests to travis.

Sad. I didn't planned to bound the tests to travis.
g1itch (Migrated from github.com) reviewed 2020-10-23 15:57:35 +02:00
src/helper_addressbook.py
g1itch (Migrated from github.com) commented 2020-10-23 15:57:35 +02:00

Why shebang here?

Why shebang here?
g1itch (Migrated from github.com) reviewed 2020-10-23 16:00:40 +02:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-23 16:00:40 +02:00

This looks like a cheat. TestProcessProto does the same: https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/tests/test_process.py#L73

This looks like a cheat. `TestProcessProto` does the same: https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/tests/test_process.py#L73
g1itch commented 2020-10-23 16:02:26 +02:00 (Migrated from github.com)
Copy Link

I don't think that increasing timeouts gonna help.

I don't think that increasing timeouts gonna help.
g1itch (Migrated from github.com) reviewed 2020-10-23 16:08:29 +02:00
src/bitmessageqt/tests/support.py
g1itch (Migrated from github.com) commented 2020-10-23 16:08:29 +02:00

core tests was designed to run from appdata. Maybe it's better to cleanup files.

core tests was designed to run from appdata. Maybe it's better to cleanup files.
g1itch (Migrated from github.com) reviewed 2020-10-23 16:14:40 +02:00
src/helper_addressbook.py
g1itch (Migrated from github.com) commented 2020-10-23 16:14:40 +02:00

Another helper ):

Another helper ):
g1itch (Migrated from github.com) reviewed 2020-10-23 16:45:57 +02:00
src/bitmessageqt/tests/support.py
g1itch (Migrated from github.com) commented 2020-10-23 16:45:57 +02:00

_translate ?
https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessageqt/support.py#L33

_translate ? https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessageqt/support.py#L33
g1itch (Migrated from github.com) reviewed 2020-10-23 16:59:15 +02:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-23 16:59:15 +02:00

I cannot find where it's used

I cannot find where it's used
g1itch (Migrated from github.com) reviewed 2020-10-23 17:02:47 +02:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-23 17:02:47 +02:00

Oh, it's from unittest.TestCase. Got it.

Oh, it's from unittest.TestCase. Got it.
g1itch (Migrated from github.com) reviewed 2020-10-23 17:04:44 +02:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-23 17:04:44 +02:00

If this hunk is platform dependent, maybe here should be some condition?

If this hunk is platform dependent, maybe here should be some condition?
g1itch (Migrated from github.com) approved these changes 2020-10-23 17:14:20 +02:00
g1itch (Migrated from github.com) left a comment

At your discretion

At your discretion
g1itch commented 2020-10-23 17:35:49 +02:00 (Migrated from github.com)
Copy Link

See more: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738338172
I'm waiting.

See more: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738338172 I'm waiting.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:17:45 +02:00
.travis.yml
PeterSurda (Migrated from github.com) commented 2020-10-23 19:17:45 +02:00

I agree but I don't think it can be fixed without a lot of refactoring.

I agree but I don't think it can be fixed without a lot of refactoring.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:17:54 +02:00
src/helper_addressbook.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:17:54 +02:00

Ok.

Ok.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:20:12 +02:00
src/helper_addressbook.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:20:12 +02:00

I mean, I agree about the shebang, but the addressbook code shouldn't be in the UI like it is now. What other options are there? Not another thread to deal with addressbook?

I mean, I agree about the shebang, but the addressbook code shouldn't be in the UI like it is now. What other options are there? Not another thread to deal with addressbook?
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:21:33 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:21:32 +02:00

Yes it's so that we have more info about the failures.

Yes it's so that we have more info about the failures.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:22:50 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:22:50 +02:00

I haven't tested it on Windows, but I assumed the result will be empty. Maybe it can be wrapped in try/except. The output is only used if there is an assertion error, it's not an assertion itself.

I haven't tested it on Windows, but I assumed the result will be empty. Maybe it can be wrapped in try/except. The output is only used if there is an assertion error, it's not an assertion itself.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:29:10 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:29:10 +02:00
  1. it isn't the same, because TestProcessProto first calls _stop_process to send a SIGTERM. This expects the program to already have closed on its own, and just send SIGKILL in an emergency.
  2. it allows you to put custom handlers into TimeoutExpired and NoSuchProcess if you want the tests to be more strict. TimeoutExpired means that there is something very wrong as it's blocking a SIGKILL.
  3. However, the main point here is line 178 as previously, it wasn't waiting for the program to finish after SIGKILL, which I think lead unpredictably to problems.
1. it isn't the same, because `TestProcessProto` first calls `_stop_process` to send a `SIGTERM`. This expects the program to already have closed on its own, and just send `SIGKILL` in an emergency. 2. it allows you to put custom handlers into `TimeoutExpired` and `NoSuchProcess` if you want the tests to be more strict. `TimeoutExpired` means that there is something very wrong as it's blocking a `SIGKILL`. 3. However, the main point here is line 178 as previously, it wasn't waiting for the program to finish after `SIGKILL`, which I think lead unpredictably to problems.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 19:44:02 +02:00
src/bitmessageqt/tests/support.py
PeterSurda (Migrated from github.com) commented 2020-10-23 19:44:02 +02:00

Maybe yes, but noone is going to do that.

Maybe yes, but noone is going to do that.
PeterSurda commented 2020-10-23 19:45:09 +02:00 (Migrated from github.com)
Copy Link

I don't think that increasing timeouts gonna help.

Noone is working on a proper fix and other work can't be done until the tests succeed.

> I don't think that increasing timeouts gonna help. Noone is working on a proper fix and other work can't be done until the tests succeed.
PeterSurda commented 2020-10-23 19:45:44 +02:00 (Migrated from github.com)
Copy Link

See more: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738338172
I'm waiting.

test_config_defaults isn't waiting for the process to finish.

> See more: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738338172 > I'm waiting. `test_config_defaults` isn't waiting for the process to finish.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 20:10:36 +02:00
src/bitmessageqt/tests/support.py
PeterSurda (Migrated from github.com) commented 2020-10-23 20:10:36 +02:00

ok

ok
g1itch (Migrated from github.com) reviewed 2020-10-23 20:56:08 +02:00
src/bitmessageqt/tests/support.py
g1itch (Migrated from github.com) commented 2020-10-23 20:56:08 +02:00

I did it in one of my rebased branches. It did not help, because cleanup is the least part of the problem.

I did it in one of my rebased branches. It did not help, because cleanup is the least part of the problem.
g1itch commented 2020-10-23 21:03:36 +02:00 (Migrated from github.com)
Copy Link

I don't think that increasing timeouts gonna help.

Noone is working on a proper fix and other work can't be done until the tests succeed.

I don't remember any shutdown tests failing in your test environment or current xenial travis setup. TestAPIShutdown failed on windows and on my recent fedora31 container.

Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954

> > I don't think that increasing timeouts gonna help. > > Noone is working on a proper fix and other work can't be done until the tests succeed. I don't remember any shutdown tests failing in your test environment or current xenial travis setup. `TestAPIShutdown` failed on windows and on my recent fedora31 container. Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954
g1itch (Migrated from github.com) reviewed 2020-10-23 21:28:00 +02:00
src/tests/test_process.py
@ -27,3 +48,4 @@
]
_files = (
'keys.dat', 'debug.log', 'messages.dat', 'knownnodes.dat',
'.api_started', 'unittest.lock'
g1itch (Migrated from github.com) commented 2020-10-23 21:27:59 +02:00

https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738338187

https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738338187
g1itch (Migrated from github.com) reviewed 2020-10-23 21:48:48 +02:00
src/helper_addressbook.py
g1itch (Migrated from github.com) commented 2020-10-23 21:48:47 +02:00

I agree that it's cool to have all CRUD tasks (on messages, addressbook and black/white lists) in the main package to use them in api, bitmessageqt and any other frontend. Also I was working on the single helper with all SQL queries in #1534 because it is bad to have SQL scattered among the code. It needs heavy testing. Currently it breaks bitmessageqt because I missed some type cast or check.

But hey, you need this helper now only because of BITMESSAGE_HOME in env and the absence of cleanup.

Another thread is the last thing I would suggest. But recently I thought about using multiprocessing for test cases which use potentially dangerous imports.

I agree that it's cool to have all CRUD tasks (on messages, addressbook and black/white lists) in the main package to use them in `api`, `bitmessageqt` and any other frontend. Also I was working on the single helper with all SQL queries in #1534 because it is bad to have SQL scattered among the code. It needs heavy testing. Currently it breaks `bitmessageqt` because I missed some type cast or check. But hey, you need this helper now only because of `BITMESSAGE_HOME` in env and the absence of cleanup. Another thread is the last thing I would suggest. But recently I thought about using multiprocessing for test cases which use potentially dangerous imports.
g1itch (Migrated from github.com) reviewed 2020-10-23 21:52:10 +02:00
src/bitmessageqt/tests/support.py
g1itch (Migrated from github.com) commented 2020-10-23 21:52:10 +02:00

Did you try this?

--- a/src/tests/test_process.py
+++ b/src/tests/test_process.py
@@ -33,6 +33,7 @@ class TestProcessProto(unittest.TestCase):
     def setUpClass(cls):
         """Setup environment and start pybitmessage"""
         cls.home = os.environ['BITMESSAGE_HOME'] = tempfile.gettempdir()
+        cls._cleanup_files()
         put_signal_file(cls.home, 'unittest.lock')
         subprocess.call(cls._process_cmd)  # nosec
         time.sleep(5)
Did you try this? ```patch --- a/src/tests/test_process.py +++ b/src/tests/test_process.py @@ -33,6 +33,7 @@ class TestProcessProto(unittest.TestCase): def setUpClass(cls): """Setup environment and start pybitmessage""" cls.home = os.environ['BITMESSAGE_HOME'] = tempfile.gettempdir() + cls._cleanup_files() put_signal_file(cls.home, 'unittest.lock') subprocess.call(cls._process_cmd) # nosec time.sleep(5) ```
PeterSurda commented 2020-10-23 23:18:09 +02:00 (Migrated from github.com)
Copy Link

I don't remember any shutdown tests failing in your test environment or current xenial travis setup. TestAPIShutdown failed on windows and on my recent fedora31 container.

I can put the timeouts back, maybe some other changes that I made fixed the issues.

Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954

Looks like pyelliptic pointers.

> I don't remember any shutdown tests failing in your test environment or current xenial travis setup. `TestAPIShutdown` failed on windows and on my recent fedora31 container. I can put the timeouts back, maybe some other changes that I made fixed the issues. > Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954 Looks like pyelliptic pointers.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 23:21:31 +02:00
src/tests/test_process.py
@ -27,3 +48,4 @@
]
_files = (
'keys.dat', 'debug.log', 'messages.dat', 'knownnodes.dat',
'.api_started', 'unittest.lock'
PeterSurda (Migrated from github.com) commented 2020-10-23 23:21:31 +02:00

That can be fixed later. We don't have a working OSX at the moment anyway.

That can be fixed later. We don't have a working OSX at the moment anyway.
PeterSurda (Migrated from github.com) reviewed 2020-10-23 23:26:21 +02:00
src/bitmessageqt/tests/support.py
PeterSurda (Migrated from github.com) commented 2020-10-23 23:26:21 +02:00

ok let me try

ok let me try
PeterSurda commented 2020-10-24 09:58:58 +02:00 (Migrated from github.com)
Copy Link

@g1itch I refactored the use of state.appdata and removed the environment variable from .travis.yml. It now passes buildbot's bionic and xenial, as well as xenial on travis-ci.org. Can you try rebasing your changes on top of it again?

@g1itch I refactored the use of `state.appdata` and removed the environment variable from `.travis.yml`. It now passes buildbot's bionic and xenial, as well as xenial on travis-ci.org. Can you try rebasing your changes on top of it again?
g1itch (Migrated from github.com) reviewed 2020-10-24 10:32:53 +02:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-24 10:32:53 +02:00

OK, I get it:

  • it doesn't disable the test because it's failing by it's own if process not stops.
  • it is not the same as super
OK, I get it: - it doesn't disable the test because it's failing by it's own if process not stops. - it is not the same as super
g1itch commented 2020-10-24 10:40:20 +02:00 (Migrated from github.com)
Copy Link

Wow! It looks like you really fixed the major issue with appdata while I was on a half way retracing it from debug -> helper_startap <-> state (loadConfig).

Wow! It looks like you really fixed the major issue with appdata while I was on a half way retracing it from debug -> helper_startap <-> state (loadConfig).
g1itch commented 2020-10-24 10:41:38 +02:00 (Migrated from github.com)
Copy Link

@g1itch I refactored the use of state.appdata and removed the environment variable from .travis.yml. It now passes buildbot's bionic and xenial, as well as xenial on travis-ci.org. Can you try rebasing your changes on top of it again?

I do the opposite: I merge it to cleaned out ci branch: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738506280

> @g1itch I refactored the use of `state.appdata` and removed the environment variable from `.travis.yml`. It now passes buildbot's bionic and xenial, as well as xenial on travis-ci.org. Can you try rebasing your changes on top of it again? I do the opposite: I merge it to cleaned out ci branch: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738506280
PeterSurda commented 2020-10-24 10:42:16 +02:00 (Migrated from github.com)
Copy Link

I do the opposite

Whichever you prefer.

> I do the opposite Whichever you prefer.
g1itch (Migrated from github.com) approved these changes 2020-10-24 11:15:05 +02:00
g1itch (Migrated from github.com) left a comment

Some formatting options and two errors on windows I found.

Some formatting options and two errors on windows I found.
src/tests/test_config.py
@ -45,6 +45,7 @@ class TestProcessConfig(TestProcessProto):
def test_config_defaults(self):
"""Test settings in the generated config"""
self._stop_process()
self._kill_process()
g1itch (Migrated from github.com) commented 2020-10-24 11:13:33 +02:00

This is like tearDown() without cleaning the files.

This is like `tearDown()` without cleaning the files.
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-24 11:09:29 +02:00

Maybe self.fail() here?

Maybe `self.fail()` here?
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-24 10:48:40 +02:00 
--- a/src/tests/test_process.py
+++ b/src/tests/test_process.py
@@ -189,9 +189,7 @@ class TestProcessShutdown(TestProcessProto):
             if cls.process.is_running():
                 cls.process.kill()
                 cls.process.wait(5)
-        except psutil.TimeoutExpired:
-            pass
-        except psutil.NoSuchProcess:
+        except (psutil.TimeoutExpired, psutil.NoSuchProcess):
             pass
         finally:
             cls._cleanup_files()
```patch --- a/src/tests/test_process.py +++ b/src/tests/test_process.py @@ -189,9 +189,7 @@ class TestProcessShutdown(TestProcessProto): if cls.process.is_running(): cls.process.kill() cls.process.wait(5) - except psutil.TimeoutExpired: - pass - except psutil.NoSuchProcess: + except (psutil.TimeoutExpired, psutil.NoSuchProcess): pass finally: cls._cleanup_files() ```
src/tests/test_process.py
@ -99,0 +170,4 @@
self.assertLessEqual(
running_threads,
self._threads_count_max,
g1itch (Migrated from github.com) commented 2020-10-24 10:44:09 +02:00

AttributeError on windows: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738506283

AttributeError on windows: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738506283
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-24 11:11:28 +02:00

https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738510444

https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738510444
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-24 10:50:22 +02:00 
except (psutil.TimeoutExpired, psutil.NoSuchProcess):
    pass
```python except (psutil.TimeoutExpired, psutil.NoSuchProcess): pass ```
g1itch commented 2020-10-24 11:26:00 +02:00 (Migrated from github.com)
Copy Link

Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954

Looks like pyelliptic pointers.

That blindsig tests failed because of openssl-1.0.1 dll. If I install PyQt4 like in winbuild.sh it will fix the tests, because pyelliptic loads openssl-1.0.1 dll bundled with pyqt in that case.

> > Windows failed until this run: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/736655954 > > Looks like pyelliptic pointers. That blindsig tests failed because of openssl-1.0.1 dll. If I install PyQt4 like in `winbuild.sh` it will fix the tests, because pyelliptic loads openssl-1.0.1 dll bundled with pyqt in that case.
PeterSurda (Migrated from github.com) reviewed 2020-10-24 11:30:33 +02:00
src/tests/test_process.py
@ -99,0 +170,4 @@
self.assertLessEqual(
running_threads,
self._threads_count_max,
PeterSurda (Migrated from github.com) commented 2020-10-24 11:30:33 +02:00

ok

ok
PeterSurda (Migrated from github.com) reviewed 2020-10-24 11:30:56 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 11:30:55 +02:00

ok, it can be split later in case you want to handle them differently

ok, it can be split later in case you want to handle them differently
PeterSurda (Migrated from github.com) reviewed 2020-10-24 11:31:04 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 11:31:04 +02:00

same as above

same as above
PeterSurda (Migrated from github.com) reviewed 2020-10-24 11:31:09 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 11:31:08 +02:00

ok

ok
g1itch (Migrated from github.com) reviewed 2020-10-24 13:15:02 +02:00
src/tests/test_process.py
@ -99,0 +139,4 @@
# because of https://github.com/giampaolo/psutil/issues/613
thread_names = subprocess.check_output([
"ps", "-L", "-o", "comm=", "--pid",
str(self.process.pid)
g1itch (Migrated from github.com) commented 2020-10-24 13:15:01 +02:00

AccessDenied on osx: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738524694

`AccessDenied` on osx: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/738524694
PeterSurda commented 2020-10-24 13:46:17 +02:00 (Migrated from github.com)
Copy Link

@g1itch Thanks for the feedback so far, I'll fix those issues.

@g1itch Thanks for the feedback so far, I'll fix those issues.
PeterSurda (Migrated from github.com) reviewed 2020-10-24 17:13:47 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 17:13:47 +02:00

_kill_process is a class method so you'd have to use cls.fail() and I don't think that would work correctly. The documentation doesn't say what happens if there are exceptions in tearDownClass, only in tearDown. Looking at unittest source, it looks like an exception in tearDownClass also adds a new error entry in the totals, just like tearDown is supposed to. So I think a more traditional exception like OSError is a better choice here. Perhaps an even better choice is to simply delete this except and let the psutil.TimeoutExpired propagate up.

`_kill_process` is a class method so you'd have to use `cls.fail()` and I don't think that would work correctly. The documentation doesn't say what happens if there are exceptions in `tearDownClass`, only in `tearDown`. Looking at [unittest source](https://github.com/python/cpython/blob/0f221d09cad46bee38d1b7a7822772df66c53028/Lib/unittest/suite.py), it looks like an exception in `tearDownClass` also adds a new error entry in the totals, just like `tearDown` is supposed to. So I think a more traditional exception like `OSError` is a better choice here. Perhaps an even better choice is to simply delete this `except` and let the `psutil.TimeoutExpired` propagate up.
PeterSurda (Migrated from github.com) reviewed 2020-10-24 17:22:08 +02:00
src/tests/test_config.py
@ -45,6 +45,7 @@ class TestProcessConfig(TestProcessProto):
def test_config_defaults(self):
"""Test settings in the generated config"""
self._stop_process()
self._kill_process()
PeterSurda (Migrated from github.com) commented 2020-10-24 17:22:08 +02:00

Yes I think you're correct.

Yes I think you're correct.
PeterSurda (Migrated from github.com) reviewed 2020-10-24 18:37:18 +02:00
src/tests/test_process.py
@ -99,0 +170,4 @@
self.assertLessEqual(
running_threads,
self._threads_count_max,
PeterSurda (Migrated from github.com) commented 2020-10-24 18:37:18 +02:00

Added an exception handler.

Added an exception handler.
PeterSurda (Migrated from github.com) reviewed 2020-10-24 18:37:26 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 18:37:25 +02:00

Done

Done
PeterSurda (Migrated from github.com) reviewed 2020-10-24 18:37:33 +02:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-24 18:37:33 +02:00

done

done
PeterSurda (Migrated from github.com) reviewed 2020-10-24 18:37:50 +02:00
src/tests/test_process.py
@ -99,0 +139,4 @@
# because of https://github.com/giampaolo/psutil/issues/613
thread_names = subprocess.check_output([
"ps", "-L", "-o", "comm=", "--pid",
str(self.process.pid)
PeterSurda (Migrated from github.com) commented 2020-10-24 18:37:49 +02:00

Test is now skipped on OSX.

Test is now skipped on OSX.
g1itch (Migrated from github.com) approved these changes 2020-10-25 12:07:36 +01:00
g1itch (Migrated from github.com) left a comment

Non-linux issues may be fixed later

Non-linux issues may be fixed later
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-25 11:45:24 +01:00

It only seems to work on linux: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738596816
Maybe also break on OSError?

It only seems to work on linux: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738596816 Maybe also break on OSError?
src/tests/test_process.py
@ -99,0 +140,4 @@
thread_names = subprocess.check_output([
"ps", "-L", "-o", "comm=", "--pid",
str(self.process.pid)
]).split()
g1itch (Migrated from github.com) commented 2020-10-25 11:49:19 +01:00

Maybe this can be shortened now:

--- a/src/tests/test_process.py
+++ b/src/tests/test_process.py
@@ -120,28 +120,11 @@ class TestProcessProto(unittest.TestCase):
             cls._cleanup_files()
 
     def _test_threads(self):
-        # only count for now
-        # because of https://github.com/giampaolo/psutil/issues/613
-        # PyBitmessage
-        #   - addressGenerator
-        #   - singleWorker
-        #   - SQL
-        #   - objectProcessor
-        #   - singleCleaner
-        #   - singleAPI
-        #   - Asyncore
-        #   - ReceiveQueue_0
-        #   - ReceiveQueue_1
-        #   - ReceiveQueue_2
-        #   - Announcer
-        #   - InvBroadcaster
-        #   - AddrBroadcaster
-        #   - Downloader
-        #   - Uploader
-
         self.longMessage = True
 
         try:
+            # using ps on Linux
+            # because of https://github.com/giampaolo/psutil/issues/613
Maybe this can be shortened now: ```patch --- a/src/tests/test_process.py +++ b/src/tests/test_process.py @@ -120,28 +120,11 @@ class TestProcessProto(unittest.TestCase): cls._cleanup_files() def _test_threads(self): - # only count for now - # because of https://github.com/giampaolo/psutil/issues/613 - # PyBitmessage - # - addressGenerator - # - singleWorker - # - SQL - # - objectProcessor - # - singleCleaner - # - singleAPI - # - Asyncore - # - ReceiveQueue_0 - # - ReceiveQueue_1 - # - ReceiveQueue_2 - # - Announcer - # - InvBroadcaster - # - AddrBroadcaster - # - Downloader - # - Uploader - self.longMessage = True try: + # using ps on Linux + # because of https://github.com/giampaolo/psutil/issues/613 ```
g1itch (Migrated from github.com) reviewed 2020-10-25 12:08:41 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-25 12:08:41 +01:00

Not so easy: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738699572

Not so easy: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738699572
PeterSurda (Migrated from github.com) reviewed 2020-10-25 12:35:18 +01:00
src/tests/test_process.py
@ -99,0 +140,4 @@
thread_names = subprocess.check_output([
"ps", "-L", "-o", "comm=", "--pid",
str(self.process.pid)
]).split()
PeterSurda (Migrated from github.com) commented 2020-10-25 12:35:17 +01:00

Yea makes sense.

Yea makes sense.
g1itch (Migrated from github.com) reviewed 2020-10-25 13:00:48 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-25 13:00:48 +01:00

Rebased in my favor: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738705081

Rebased in my favor: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738705081
g1itch (Migrated from github.com) reviewed 2020-10-26 11:20:51 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-26 11:20:51 +01:00 
commit 77680a2ef4a9dd16736960e457529a66d7a6856d
Author: Dmitri Bogomolov <4glitch@gmail.com>
Date:   Mon Oct 26 07:57:52 2020 +0200

    Fix test_threads for non-linux environment:
    
     - check found thread_names adequacy, use num_threads()
     - take into account one extra thread on windows

diff --git a/src/tests/test_process.py b/src/tests/test_process.py
index cd114323..63aa2e9c 100644
--- a/src/tests/test_process.py
+++ b/src/tests/test_process.py
@@ -12,8 +12,6 @@ import unittest
 
 import psutil
 
-from sys import platform
-
 from pybitmessage.paths import set_appdata_folder
 
 
@@ -126,49 +124,36 @@ class TestProcessProto(unittest.TestCase):
             cls._cleanup_files()
 
     def _test_threads(self):
-        # only count for now
-        # because of https://github.com/giampaolo/psutil/issues/613
-        # PyBitmessage
-        #   - addressGenerator
-        #   - singleWorker
-        #   - SQL
-        #   - objectProcessor
-        #   - singleCleaner
-        #   - singleAPI
-        #   - Asyncore
-        #   - ReceiveQueue_0
-        #   - ReceiveQueue_1
-        #   - ReceiveQueue_2
-        #   - Announcer
-        #   - InvBroadcaster
-        #   - AddrBroadcaster
-        #   - Downloader
-        #   - Uploader
-
         self.longMessage = True
 
         try:
-            thread_names = \
-                subprocess.check_output(["ps", "-L", "-o", "comm=",
-                                         "--pid",
-                                         str(self.process.pid)]).split()
-        # pylint: disable=broad-except
-        except BaseException:
+            # using ps for posix platforms
+            # because of https://github.com/giampaolo/psutil/issues/613
+            thread_names = subprocess.check_output([
+                "ps", "-L", "-o", "comm=", "--pid",
+                str(self.process.pid)
+            ]).split()
+        except:  # pylint: disable=broad-except
             thread_names = []
 
-        extra_threads = []
-        missing_threads = []
-        for thread_name in thread_names:
-            if thread_name not in self._threads_names:
-                extra_threads.append(thread_name)
-        for thread_name in self._threads_names:
-            if thread_name not in thread_names:
-                missing_threads.append(thread_name)
-
-        msg = "Missing threads: {}, Extra threads: {}".format(
-            ",".join(missing_threads), ",".join(extra_threads))
-
-        running_threads = len(self.process.threads())
+        running_threads = len(thread_names)
+        if 0 < running_threads < 30:  # adequacy check
+            extra_threads = []
+            missing_threads = []
+            for thread_name in thread_names:
+                if thread_name not in self._threads_names:
+                    extra_threads.append(thread_name)
+            for thread_name in self._threads_names:
+                if thread_name not in thread_names:
+                    missing_threads.append(thread_name)
+
+            msg = "Missing threads: {}, Extra threads: {}".format(
+                ",".join(missing_threads), ",".join(extra_threads))
+        else:
+            running_threads = self.process.num_threads()
+            if sys.platform.startswith('win'):
+                running_threads -= 1  # one extra thread on Windows!
+            msg = "Unexpected running thread count"
 
         self.assertGreaterEqual(
             running_threads,
@@ -220,7 +205,6 @@ class TestProcess(TestProcessProto):
                 'Failed to read file %s' % pfile
             )
 
-    @unittest.skipIf(platform.startswith('darwin'), "Doesn't work on OSX")
     def test_threads(self):
         """Testing PyBitmessage threads"""
         self._test_threads()
```patch commit 77680a2ef4a9dd16736960e457529a66d7a6856d Author: Dmitri Bogomolov <4glitch@gmail.com> Date: Mon Oct 26 07:57:52 2020 +0200 Fix test_threads for non-linux environment: - check found thread_names adequacy, use num_threads() - take into account one extra thread on windows diff --git a/src/tests/test_process.py b/src/tests/test_process.py index cd114323..63aa2e9c 100644 --- a/src/tests/test_process.py +++ b/src/tests/test_process.py @@ -12,8 +12,6 @@ import unittest import psutil -from sys import platform - from pybitmessage.paths import set_appdata_folder @@ -126,49 +124,36 @@ class TestProcessProto(unittest.TestCase): cls._cleanup_files() def _test_threads(self): - # only count for now - # because of https://github.com/giampaolo/psutil/issues/613 - # PyBitmessage - # - addressGenerator - # - singleWorker - # - SQL - # - objectProcessor - # - singleCleaner - # - singleAPI - # - Asyncore - # - ReceiveQueue_0 - # - ReceiveQueue_1 - # - ReceiveQueue_2 - # - Announcer - # - InvBroadcaster - # - AddrBroadcaster - # - Downloader - # - Uploader - self.longMessage = True try: - thread_names = \ - subprocess.check_output(["ps", "-L", "-o", "comm=", - "--pid", - str(self.process.pid)]).split() - # pylint: disable=broad-except - except BaseException: + # using ps for posix platforms + # because of https://github.com/giampaolo/psutil/issues/613 + thread_names = subprocess.check_output([ + "ps", "-L", "-o", "comm=", "--pid", + str(self.process.pid) + ]).split() + except: # pylint: disable=broad-except thread_names = [] - extra_threads = [] - missing_threads = [] - for thread_name in thread_names: - if thread_name not in self._threads_names: - extra_threads.append(thread_name) - for thread_name in self._threads_names: - if thread_name not in thread_names: - missing_threads.append(thread_name) - - msg = "Missing threads: {}, Extra threads: {}".format( - ",".join(missing_threads), ",".join(extra_threads)) - - running_threads = len(self.process.threads()) + running_threads = len(thread_names) + if 0 < running_threads < 30: # adequacy check + extra_threads = [] + missing_threads = [] + for thread_name in thread_names: + if thread_name not in self._threads_names: + extra_threads.append(thread_name) + for thread_name in self._threads_names: + if thread_name not in thread_names: + missing_threads.append(thread_name) + + msg = "Missing threads: {}, Extra threads: {}".format( + ",".join(missing_threads), ",".join(extra_threads)) + else: + running_threads = self.process.num_threads() + if sys.platform.startswith('win'): + running_threads -= 1 # one extra thread on Windows! + msg = "Unexpected running thread count" self.assertGreaterEqual( running_threads, @@ -220,7 +205,6 @@ class TestProcess(TestProcessProto): 'Failed to read file %s' % pfile ) - @unittest.skipIf(platform.startswith('darwin'), "Doesn't work on OSX") def test_threads(self): """Testing PyBitmessage threads""" self._test_threads() ```
g1itch (Migrated from github.com) reviewed 2020-10-26 14:48:28 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-26 14:48:28 +01:00

Almost clear: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738955127

Almost clear: https://travis-ci.org/github/g1itch/PyBitmessage/builds/738955127
PeterSurda (Migrated from github.com) reviewed 2020-10-26 17:43:42 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-26 17:43:42 +01:00

I added os.path.exists.

I added `os.path.exists`.
PeterSurda (Migrated from github.com) reviewed 2020-10-26 17:43:58 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-26 17:43:57 +01:00

I added this now.

I added this now.
g1itch (Migrated from github.com) approved these changes 2020-10-27 12:29:16 +01:00
g1itch (Migrated from github.com) left a comment

TImeout expired every time on osx: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739252746

TImeout expired every time on osx: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739252746
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 12:25:52 +01:00

87 char line

87 char line
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 12:13:48 +01:00

if not os.path.exists it will be full wait until pybitmessage -t stops. See:
https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessagemain.py#L180
https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessagemain.py#L371

if not `os.path.exists` it will be full wait until `pybitmessage -t` stops. See: https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessagemain.py#L180 https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/bitmessagemain.py#L371
g1itch (Migrated from github.com) reviewed 2020-10-27 12:32:52 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 12:32:52 +01:00

Hmm https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739258079

Hmm https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739258079
g1itch (Migrated from github.com) reviewed 2020-10-27 12:57:34 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 12:57:34 +01:00

no fractional part in st_mtime: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739262652

no fractional part in `st_mtime`: https://travis-ci.org/github/g1itch/PyBitmessage/jobs/739262652
g1itch (Migrated from github.com) reviewed 2020-10-27 14:59:06 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 14:59:06 +01:00

maybe int(time.time())?

maybe `int(time.time())`?
g1itch (Migrated from github.com) reviewed 2020-10-27 15:02:04 +01:00
src/tests/test_process.py
g1itch (Migrated from github.com) commented 2020-10-27 15:02:04 +01:00

As I can see OSError raised when os.path.exists(os.path.join(cls.home, 'singleton.lock')) is False.

As I can see `OSError` raised when `os.path.exists(os.path.join(cls.home, 'singleton.lock')) is False`.
g1itch (Migrated from github.com) reviewed 2020-10-27 15:12:54 +01:00
src/tests/test_process.py
@ -70,4 +122,3 @@
try:
if not cls._stop_process():
print(open(os.path.join(cls.home, 'debug.log'), 'rb').read())
cls.process.kill()
g1itch (Migrated from github.com) commented 2020-10-27 15:12:54 +01:00

Why sleep 10 seconds if singleton.lock is already checked? What may happen?

Why sleep 10 seconds if singleton.lock is already checked? What may happen?
g1itch commented 2020-10-27 16:28:32 +01:00 (Migrated from github.com)
Copy Link

No handlers could be found for logger "default": https://travis-ci.org/github/Bitmessage/PyBitmessage/builds/739215140#L1084

No handlers could be found for logger "default": https://travis-ci.org/github/Bitmessage/PyBitmessage/builds/739215140#L1084
g1itch (Migrated from github.com) reviewed 2020-10-27 16:32:00 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-27 16:32:00 +01:00

I don't understand the purpose, it's not used in code:

$ grep -r 'getLogger' src | grep root
src/tests/test_logger.py:        logger2 = logging.getLogger('root')
I don't understand the purpose, it's not used in code: ```shell $ grep -r 'getLogger' src | grep root src/tests/test_logger.py: logger2 = logging.getLogger('root') ```
g1itch (Migrated from github.com) reviewed 2020-10-27 16:32:24 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-27 16:32:24 +01:00

Why only on second pass?

Why only on second pass?
g1itch (Migrated from github.com) reviewed 2020-10-28 08:44:01 +01:00
src/tests/test_process.py
@ -70,4 +122,3 @@
try:
if not cls._stop_process():
print(open(os.path.join(cls.home, 'debug.log'), 'rb').read())
cls.process.kill()
g1itch (Migrated from github.com) commented 2020-10-28 08:44:01 +01:00

OK, I see. It is to start all threads and not fail test_threads and test_shutdown.

OK, I see. It is to start all threads and not fail test_threads and test_shutdown.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:54:52 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-28 08:54:52 +01:00

Ok I'll look throught this.

Ok I'll look throught this.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:55:34 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-28 08:55:34 +01:00

I see.

I see.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:55:43 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-28 08:55:43 +01:00

Ok makes sense.

Ok makes sense.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:55:51 +01:00
src/tests/test_process.py
PeterSurda (Migrated from github.com) commented 2020-10-28 08:55:51 +01:00

Oh, now I get it.

Oh, now I get it.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:56:16 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
PeterSurda (Migrated from github.com) commented 2020-10-28 08:56:16 +01:00

I think it may be leftover from debugging, perhaps it isn't necessary anymore.

I think it may be leftover from debugging, perhaps it isn't necessary anymore.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 08:57:00 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
PeterSurda (Migrated from github.com) commented 2020-10-28 08:57:00 +01:00

I don't understand the logic flow of the passes, this may be a leftover from debugging and perhaps isn't necesasry anymore.

I don't understand the logic flow of the passes, this may be a leftover from debugging and perhaps isn't necesasry anymore.
PeterSurda commented 2020-10-28 08:57:12 +01:00 (Migrated from github.com)
Copy Link

I'll work on the requested changes.

I'll work on the requested changes.
g1itch (Migrated from github.com) reviewed 2020-10-28 08:57:36 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-28 08:57:36 +01:00

This not fixes the test. I'll describe how it works, how it worked and how I wanted it to work a bit later.

This not fixes the test. I'll describe how it works, how it worked and how I wanted it to work a bit later.
PeterSurda (Migrated from github.com) reviewed 2020-10-28 09:12:11 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
PeterSurda (Migrated from github.com) commented 2020-10-28 09:12:11 +01:00

I'll look at it.

I'll look at it.
g1itch (Migrated from github.com) reviewed 2020-10-28 11:16:15 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-28 11:16:14 +01:00

I don't understand the logic flow of the passes, this may be a leftover from debugging and perhaps isn't necesasry anymore.

A proper test should not rely on other tests side effects.

What I wanted to demonstrate in this particular test:

  1. when you import logger from debug it sets up the logging based on logging.dat config
  2. logger = logging.getLogger('default') set after the import will be the same as in debug
  3. if you call resetLogging() the configuration will be reapplied

All that works only after the first import of debug because it calls helper_startup.loadConfig().

How it worked before:

  1. precondition: there is no other test importing debug or other module inducing config setup or appdata change
  2. first pass detected by debug.log in appdata remaining from core tests
  3. on the second pass you need resetLogging()

You can reveal it by replacing all tempfile.gettempdir() by tempfile.mkdtemp() as was suggested before.

> I don't understand the logic flow of the passes, this may be a leftover from debugging and perhaps isn't necesasry anymore. A proper test should not rely on other tests side effects. What I wanted to demonstrate in this particular test: 1. when you import `logger` from `debug` it sets up the logging based on `logging.dat` config 1. `logger = logging.getLogger('default')` set after the import will be the same as in `debug` 1. if you call `resetLogging()` the configuration will be reapplied All that works only after the first import of `debug` because it calls `helper_startup.loadConfig()`. How it worked before: 1. precondition: there is no other test importing `debug` or other module inducing config setup or appdata change 1. first pass detected by `debug.log` in appdata remaining from core tests 1. on the second pass you need `resetLogging()` You can reveal it by replacing all `tempfile.gettempdir()` by `tempfile.mkdtemp()` as was suggested before.
g1itch (Migrated from github.com) reviewed 2020-10-28 11:21:15 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-28 11:21:14 +01:00

I'm not sure that I can precisely reconstruct what's happening now. It places logging.dat which is used by subsequent runs of pybitmessage process and can be detected on second pass.

I'm not sure that I can precisely reconstruct what's happening now. It places `logging.dat` which is used by subsequent runs of pybitmessage process and can be detected on second pass.
g1itch (Migrated from github.com) reviewed 2020-10-28 11:26:34 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-28 11:26:34 +01:00

Maybe subclass this test case from TestProcessProto and check only the presence of pattern (severity and initial message)? Then we can replace logging.getLogger('default') by conventional logging.getLogger(__name__), compose proper logging.dat and check presence of all submodule names which write logs.

Maybe subclass this test case from `TestProcessProto` and check only the presence of pattern (severity and initial message)? Then we can replace `logging.getLogger('default')` by conventional `logging.getLogger(__name__)`, compose proper `logging.dat` and check presence of all submodule names which write logs.
g1itch (Migrated from github.com) reviewed 2020-10-28 11:50:42 +01:00
src/tests/test_process.py
@ -70,4 +122,3 @@
try:
if not cls._stop_process():
print(open(os.path.join(cls.home, 'debug.log'), 'rb').read())
cls.process.kill()
g1itch (Migrated from github.com) commented 2020-10-28 11:50:42 +01:00

But I hope that 5 sec will be enough for any environment (on travis it works without delay). This delay is used before starting tests in any test case subclassed from TestProcessProto on each pass.

But I hope that 5 sec will be enough for any environment (on travis it works without delay). This delay is used before starting tests in any test case subclassed from `TestProcessProto` on each pass.
g1itch commented 2020-10-28 14:23:57 +01:00 (Migrated from github.com)
Copy Link

I'm waiting to check the new approach: https://travis-ci.org/github/g1itch/PyBitmessage/builds/739605155

I'm waiting to check the new approach: https://travis-ci.org/github/g1itch/PyBitmessage/builds/739605155
g1itch commented 2020-10-28 15:42:45 +01:00 (Migrated from github.com)
Copy Link

The cleanup of the core tests is here.

The cleanup of the core tests is [here](https://github.com/Bitmessage/PyBitmessage/blob/v0.6/src/tests/core.py#L51).
g1itch commented 2020-10-28 21:03:28 +01:00 (Migrated from github.com)
Copy Link

xenial, bionic and fedora31: OK, small hangup on focal, waiting for windows

https://travis-ci.org/github/g1itch/PyBitmessage/builds/739657803 - process based TestLogger and cleanup tweaks.

xenial, bionic and fedora31: OK, small hangup on focal, waiting for windows https://travis-ci.org/github/g1itch/PyBitmessage/builds/739657803 - process based `TestLogger` and cleanup tweaks.
PeterSurda (Migrated from github.com) reviewed 2020-10-30 10:00:44 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
PeterSurda (Migrated from github.com) commented 2020-10-30 10:00:44 +01:00

first pass detected by debug.log in appdata remaining from core tests

Only if it uses the same config directory. But the tests change the location of the config directory. I think this breaks the logic. You can see now that I reverted to your original logic in test_logger.py, and it fails on travis-ci.org. I don't want to merge until this is resolved.

Oh it worked. So maybe I don't understand it correctly.

> first pass detected by debug.log in appdata remaining from core tests ~~Only if it uses the same config directory. But the tests change the location of the config directory. I think this breaks the logic. You can see now that I reverted to your original logic in `test_logger.py`, and it fails on travis-ci.org. I don't want to merge until this is resolved.~~ Oh it worked. So maybe I don't understand it correctly.
PeterSurda commented 2020-10-30 10:03:19 +01:00 (Migrated from github.com)
Copy Link

I think all your feedback has been applied, and travis-ci passes. I think the rest of the issue can be resolved later, and this is ok to merge now. Let me know if you agree.

I think all your feedback has been applied, and travis-ci passes. I think the rest of the issue can be resolved later, and this is ok to merge now. Let me know if you agree.
g1itch (Migrated from github.com) reviewed 2020-10-30 10:04:20 +01:00
src/tests/test_logger.py
@ -35,11 +37,14 @@ level=DEBUG
handlers=default
'''
g1itch (Migrated from github.com) commented 2020-10-30 10:04:20 +01:00

I wrote before that my original logic was wrong. Standalone tests should not import any module with side effects.
I debug my process-based variant, it succedes only on linux: https://travis-ci.org/github/g1itch/PyBitmessage/builds/740134631

I wrote before that my original logic was wrong. Standalone tests should not import any module with side effects. I debug my process-based variant, it succedes only on linux: https://travis-ci.org/github/g1itch/PyBitmessage/builds/740134631
g1itch commented 2020-10-30 10:12:14 +01:00 (Migrated from github.com)
Copy Link

I think all your feedback has been applied, and travis-ci passes. I think the rest of the issue can be resolved later, and this is ok to merge now. Let me know if you agree.

Now I don't see any help from get_active_config_folder() and related code.

> I think all your feedback has been applied, and travis-ci passes. I think the rest of the issue can be resolved later, and this is ok to merge now. Let me know if you agree. Now I don't see any help from `get_active_config_folder()` and related code.
PeterSurda commented 2020-10-30 10:20:23 +01:00 (Migrated from github.com)
Copy Link

Now I don't see any help from get_active_config_folder() and related code.

It works on xenial on buildbot now.

> Now I don't see any help from get_active_config_folder() and related code. It works on xenial on buildbot now.
PeterSurda commented 2020-10-30 10:22:44 +01:00 (Migrated from github.com)
Copy Link

So what now?

So what now?
g1itch commented 2020-10-30 10:25:54 +01:00 (Migrated from github.com)
Copy Link

Now I don't see any help from get_active_config_folder() and related code.

It works on xenial on buildbot now.

But it you remove initial src/bitmessagemain.py -t it may stop working.

> > Now I don't see any help from get_active_config_folder() and related code. > > It works on xenial on buildbot now. But it you remove initial `src/bitmessagemain.py -t` it may stop working.
PeterSurda commented 2020-10-30 10:32:13 +01:00 (Migrated from github.com)
Copy Link

Well you can fix it later.

Well you can fix it later.
PeterSurda commented 2020-10-30 10:57:57 +01:00 (Migrated from github.com)
Copy Link

So apart from the logger do you have any other complaints?

So apart from the logger do you have any other complaints?
g1itch commented 2020-10-30 11:29:18 +01:00 (Migrated from github.com)
Copy Link

So apart from the logger do you have any other complaints?

While this version TestLogger is here any newly written test case may fail unexpectedly.

get_active_config_folder() code is doing nothing.

Also I have one more assertion which should not fail. I'll prepare another draft PR so you can test it on your buildbot.

> So apart from the logger do you have any other complaints? While this version `TestLogger` is here any newly written test case may fail unexpectedly. `get_active_config_folder()` code is doing nothing. Also I have one more assertion which should not fail. I'll prepare another draft PR so you can test it on your buildbot.
PeterSurda commented 2020-10-30 11:37:19 +01:00 (Migrated from github.com)
Copy Link

get_active_config_folder() code is doing nothing.

It fixes the test_config.py errors. The logger tests can be fixed in a separate PR.

> get_active_config_folder() code is doing nothing. It fixes the `test_config.py` errors. The logger tests can be fixed in a separate PR.
g1itch commented 2020-10-30 11:43:06 +01:00 (Migrated from github.com)
Copy Link

get_active_config_folder() code is doing nothing.

It fixes the test_config.py errors. The logger tests can be fixed in a separate PR.

What fixes test_config is proper cleanup and guaranteed kill in tearDownClass - that's what I'm going to demonstrate in the new rebased branch.

> > get_active_config_folder() code is doing nothing. > > It fixes the `test_config.py` errors. The logger tests can be fixed in a separate PR. What fixes `test_config` is proper cleanup and guaranteed kill in `tearDownClass` - that's what I'm going to demonstrate in the new rebased branch.
PeterSurda commented 2020-10-30 11:54:43 +01:00 (Migrated from github.com)
Copy Link

Ok then I close this and you can make a PR with your changes.

Ok then I close this and you can make a PR with your changes.
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
g1itch
Labels
Clear labels   
bug

Something isn't working as it's supposed to

  
build

Related to building or build system

  
dependencies

Pull requests that update a dependency file

  
developers

The issue is relevant mainly for developers rather than users

  
documentation

   
duplicate

   
enhancement

New feature

  
formatting

Code or UI text formatting

  
invalid

   
legal

   
mobile

Frontend for mobile devices (kivy)

  
obsolete

   
packaging

   
performance

   
protocol

Protocol change

  
question

   
refactoring

   
regression

Some issue, fixed in the past, appears again

  
security

   
test

   
translation

   
usability

   
wontfix
No Label
bug
build
dependencies
developers
documentation
duplicate
enhancement
formatting
invalid
legal
mobile
obsolete
packaging
performance
protocol
question
refactoring
regression
security
test
translation
usability
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Bitmessage/PyBitmessage-2024-08-21#1683
No description provided.
Delete Branch "teardown-test1"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?