Add tests for a variety of util functions #2496

forslund · 2020-03-09T13:07:39Z

Description

Add test cases
Logic change in resolve_resource_file to make it work as the docstring indicated.
split the __init__.py into subfiles based on functionality.

This now changes the read_stripped_lines to a generator. This changes the return type and may be considered breaking but it'll rarely be used indexed and there's currently nothing that's using the function as far as I've been able to determine.

How to test

Ensure that mycroft is still functioning and that tests passes.

Contributor license agreement signed?

CLA [ Yes ]

clusterfudge

Thanks for adding tests!

clusterfudge · 2020-03-10T03:35:44Z

mycroft/util/file_utils.py

+    return None  # Resource cannot be resolved
+
+
+def read_stripped_lines(filename):


This seems like an opportunity to optimize this to use a generator.

Would that be considered a non-breaking change? You can't index a generator...but it's quite a small break so maybe it's ok...

At the very least I can add a todo comment

clusterfudge · 2020-03-10T03:42:07Z

mycroft/util/network_utils.py

+    Returns:
+        True if internet connection can be detected
+    """
+    if _connected_dns():


It seems to me like the dns check is sufficient, and all the checks are potentially DoS and information disclosure concerns.

See here for an example of this type of problem in the wild.

The checks have grown a bit over time. The dns lookup doesn't work on all networks due to filtering, hence ncis check...

The google check was added to handle some weird edge cases filtering out Mark-1's where the AP wasn't shut off completely. It should be removed at some point

yeah, I guess I'm just not fully understanding why we'd use both domains here. Outbound port filtering makes sense for DNS vs HTTP protocols, I guess we're trying another tactic with http/https ports?

A "good internet citizen" would probably try to connect to a mycroft-operated or public for-this-purpose endpoint. NCSI seems to hit that (if intended for msft users), but dns and google.com don't strike me as good choices here.

mycroft/util/string_utils.py

clusterfudge · 2020-03-10T03:48:43Z

test/unittests/util/test_time.py

+
+
+@mock.patch('mycroft.configuration.Configuration')
+class TestTimeFuncs(TestCase):


future improvement: A more common strategy to testing time is to introduce the concept of a Clock as an interface, and provide deterministic clocks at test time. This prevents the necessity of mocking global statics, which can cause cascading failures through tests (and introduce bugs!) when misused.

forslund · 2020-03-11T15:10:30Z

Updated marking the redundant string functions as to be removed and changed the read_stripped_lines to a generator as suggested.

test/unittests/util/test_util.py

chrisveilleux

If we are going to take the time to refactor some of this code, let's get after it!

mycroft/util/audio_utils.py

mycroft/util/file_utils.py

mycroft/util/process_utils.py

test/unittests/util/test_time.py

chrisveilleux · 2020-03-27T02:04:50Z

test/unittests/util/test_util.py

+from unittest import TestCase, mock
+
+from mycroft import MYCROFT_ROOT_PATH
+from mycroft.util import (resolve_resource_file, curate_cache,


convert to new directory structure (e.g. from mycroft.util.audio_util, etc.). should these tests be broken up into one file per module being tested?

mycroft.util should still export all these methods so I think it's better for the test to pull from the module and not the submodules

forslund · 2020-03-28T10:22:44Z

I didn't originally intend to do too much changes to the functions themselves, just organize them a bit better but I'll continue the refactoring and clean up the code as much as I get time to this weekend.

chrisveilleux · 2020-03-28T17:33:55Z

I didn't originally intend to do too much changes to the functions themselves, just organize them a bit better but I'll continue the refactoring and clean up the code as much as I get time to this weekend.

Understood, but this is how we make the code better, by chipping away at it like this. If we don't make these changes as we touch the code, when do we?

forslund · 2020-03-28T17:59:12Z

The next time? I have this as an ongoing weekend morning project, chipping away pushing PR's at logical intervals.

However now I've almost completed the second pass of this so I can just as well add ot to this PR even if it makes the reviewing much more involved

devops-mycroft · 2020-04-11T07:06:58Z

Voight Kampff Integration Test Succeeded (Results)

Comment says the folder to be checked should be datadir/res, this makes the code mean the same thing.

Move utils away from the __init__.py file

- create helper functions for getting file stats and removing files in order of age - create wrapper function for turning MB into bytes

- Remove superfluous parentheses - Remove unneccessary pass

ensure_directory_exists() now takes the directory permissions as an optional argument.

Adds comments for PULSE ENVIRONMENT

devops-mycroft · 2020-04-18T07:33:34Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-04-27T07:56:26Z

Voight Kampff Integration Test Succeeded (Results)

forslund · 2020-04-27T08:47:38Z

I did a final update to the Logging so the known error (FileNotFoundError) will log a short informative message while an unknown error (Exception) will show the stack trace.

devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Mar 9, 2020

forslund force-pushed the test/util branch from 4347efe to 2589784 Compare March 9, 2020 13:23

clusterfudge reviewed Mar 10, 2020

View reviewed changes

forslund force-pushed the test/util branch from 2589784 to c348115 Compare March 11, 2020 14:54

forslund requested review from chrisveilleux and krisgesling March 26, 2020 06:56

krisgesling reviewed Mar 26, 2020

View reviewed changes

test/unittests/util/test_util.py Outdated Show resolved Hide resolved

forslund mentioned this pull request Mar 26, 2020

Add configurable API url to Watson TTS #2514

Merged

1 task

forslund force-pushed the test/util branch from c348115 to d9eb6cc Compare March 26, 2020 07:33

forslund added Type: Refactoring and other improvements Improvement of code and documentation that does not alter functionality. type: test labels Mar 26, 2020

chrisveilleux suggested changes Mar 27, 2020

View reviewed changes

forslund force-pushed the test/util branch 2 times, most recently from 165fbbe to 4582597 Compare March 29, 2020 13:46

forslund force-pushed the test/util branch 3 times, most recently from 3795f0a to 5a6d4b6 Compare April 11, 2020 06:20

forslund added 8 commits April 17, 2020 23:42

Add test for time utils

c52f283

Update docstrings of mycroft/util/__init__.py

b991453

Correct the datdir/res path

7e88ae7

Comment says the folder to be checked should be datadir/res, this makes the code mean the same thing.

Add tests for cache and file functions.

f872daf

Add tests for audio playback functions

2582d78

Refactor util library.

71683d1

Move utils away from the __init__.py file

Add tests for string related utils

76ff53a

Add tests for reading file functions

0d79af8

forslund added 17 commits April 17, 2020 23:42

Mark redunant string_utils as to be removed

a0709c8

Replace read_stripped_lines with generator

60c0af6

Simplify record function

e3f4d22

Simplify and clean up curate_cache

82e3500

- create helper functions for getting file stats and removing files in order of age - create wrapper function for turning MB into bytes

Correct some docstrings

b0be6a7

Split tests into several files

e218533

Refactor test change loglevel and add tests for process_utils

aa42552

Add simple test for get_platform

05695f5

Minor cleanups

26ef052

- Remove superfluous parentheses - Remove unneccessary pass

Reuse ensure_directory_exists() in create_file()

06157e3

ensure_directory_exists() now takes the directory permissions as an optional argument.

Minor cleanup of download docstrings and imports

3a908a1

Add basic test for download utility

a3c625b

Improve getting tmp download path

c5dccc1

Improve documentation of download util

313aaa6

Update time util docstrings

707be52

Refactor play_* functions to use a common base

cf40c2e

Adds comments for PULSE ENVIRONMENT

Add test for record function

e651e39

forslund force-pushed the test/util branch from 5a6d4b6 to 707be52 Compare April 17, 2020 21:43

MycroftAI deleted a comment from devops-mycroft Apr 18, 2020

chrisveilleux approved these changes Apr 21, 2020

View reviewed changes

Improve audio utils log

1258480

forslund merged commit bcb426a into MycroftAI:dev Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for a variety of util functions #2496

Add tests for a variety of util functions #2496

forslund commented Mar 9, 2020 •

edited

Loading

clusterfudge left a comment

clusterfudge Mar 10, 2020

forslund Mar 10, 2020

clusterfudge Mar 10, 2020

forslund Mar 10, 2020

clusterfudge Mar 10, 2020

clusterfudge Mar 10, 2020

forslund commented Mar 11, 2020

chrisveilleux left a comment

chrisveilleux Mar 27, 2020

forslund Mar 27, 2020

forslund commented Mar 28, 2020

chrisveilleux commented Mar 28, 2020

forslund commented Mar 28, 2020

devops-mycroft commented Apr 11, 2020

devops-mycroft commented Apr 18, 2020

devops-mycroft commented Apr 27, 2020

forslund commented Apr 27, 2020

		return None # Resource cannot be resolved


		def read_stripped_lines(filename):



		@mock.patch('mycroft.configuration.Configuration')
		class TestTimeFuncs(TestCase):

Add tests for a variety of util functions #2496

Add tests for a variety of util functions #2496

Conversation

forslund commented Mar 9, 2020 • edited Loading

Description

How to test

Contributor license agreement signed?

clusterfudge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forslund commented Mar 11, 2020

chrisveilleux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forslund commented Mar 28, 2020

chrisveilleux commented Mar 28, 2020

forslund commented Mar 28, 2020

devops-mycroft commented Apr 11, 2020

devops-mycroft commented Apr 18, 2020

devops-mycroft commented Apr 27, 2020

forslund commented Apr 27, 2020

forslund commented Mar 9, 2020 •

edited

Loading