port/tests_from_core #168
port/tests_from_core #168
Conversation
WalkthroughThe changes in this pull request involve modifications to several configuration files related to TTS (text-to-speech) settings across different languages, including the removal of specific configurations. Additionally, new configuration files for the Mycroft system in Brazilian Portuguese and other formats (JSON, YAML) have been introduced. A copyright notice was added to a test file, and a suite of unit tests for configuration management was created. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
test/unittests/configuration/test_configuration.py (3)
1-10: Address the TODO comment about moving the test class.The TODO comment suggests moving this test class to ovos-config. This would improve the organization and maintainability of the test suite.
Would you like assistance in creating a GitHub issue to track the task of moving this test class to ovos-config?
32-62: LGTM! Comprehensive test for LocalConf with a minor suggestion.The
test_localmethod thoroughly tests theLocalConfclass, covering:
- Initialization with mocked file operations and JSON loading.
- Merging of configurations.
- Storing configurations.
- Edge cases like non-existent files and directories.
This approach ensures robust testing of the
LocalConfclass under various scenarios.Consider adding a test case for merging nested dictionaries to ensure deep merging works correctly. This would further improve the test coverage.
64-81: LGTM! Thorough test for file format compatibility with a suggestion.The
test_file_formatsmethod effectively tests the compatibility between YAML and JSON configurations by:
- Loading configurations from both formats and comparing them.
- Exporting configurations to both formats and verifying the results.
- Ensuring that the exported configurations match the original ones.
This approach guarantees that configurations can be seamlessly used across different file formats.
Consider adding assertions to verify the content of the exported files, not just their existence. This could involve reading the exported files and comparing their content with the original configurations.
test/unittests/configuration/mycroft.yml (10)
1-12: Consider resource implications of multiple active audio backends.The configuration enables multiple audio backends (OCP, simple, and VLC) simultaneously. While this provides flexibility, it might impact system resources. Consider evaluating the necessity of having all backends active by default and potentially disabling unused ones to optimize resource usage.
13-30: Reconsider cache path and debug settings.
The cache path is set to a temporary directory (
/tmp/mycroft/cache). This may lead to data loss upon system reboot. Consider using a more persistent location for the cache.Debug mode is set to false, which is suitable for production. However, for testing and troubleshooting purposes, you might want to create a separate configuration or implement a mechanism to easily toggle debug mode.
30-48: Ensure consistency in wake word engines and review thresholds.
The configuration uses different wake word engines for "hey mycroft" (Precise) and "wake up" (Pocketsphinx). Consider using the same engine for both to ensure consistent behavior and easier maintenance.
The threshold for "wake up" (1.0e-20) seems very low compared to "hey mycroft" (1.0e-90). This might lead to frequent false positives. Consider adjusting this threshold to a more appropriate value to balance sensitivity and accuracy.
53-81: Validate VAD settings and review recording timeout.
The VAD settings are comprehensive, but they might need fine-tuning based on real-world performance. Consider implementing a mechanism to easily adjust these settings during testing phases.
The
recording_timeoutis set to 10 seconds, which might be too long for some use cases and could lead to unnecessary processing of silence. Consider reducing this timeout or implementing a dynamic timeout based on detected speech activity.
82-99: Make location and timezone configurable.The current configuration hardcodes the location to Lawrence, Kansas, USA, and the timezone to Central Standard Time. For a more flexible and user-friendly system, consider:
- Implementing a mechanism to automatically detect the user's location and timezone.
- Allowing users to manually set their location and timezone through a settings interface.
- Using a location service API to fetch accurate location data based on IP address or user input.
This approach would make the system more adaptable to different user locations and preferences.
100-105: Enhance network configuration flexibility.
The current configuration uses Google's public DNS servers. While these are reliable, they might not be suitable for all deployment scenarios. Consider making the DNS server addresses configurable or using the system's default DNS settings.
The NCSI (Network Connectivity Status Indicator) endpoint is set to a Microsoft URL. This might not be reliable for all network configurations. Consider:
- Making the NCSI endpoint configurable.
- Implementing multiple connectivity check methods (e.g., ping, HTTP GET) to various reliable endpoints.
- Allowing users to define their own connectivity check endpoints.
These changes would make the system more adaptable to different network environments and user preferences.
130-153: Enhance skill management and security.
The
blacklisted_skillsarray is empty. Consider implementing a mechanism to easily blacklist skills that might pose security risks or cause performance issues. This could include:
- A predefined list of known problematic skills.
- A user-configurable blacklist.
- An automated system to detect and blacklist misbehaving skills.
The priority skills (mycroft-pairing and mycroft-volume) seem appropriate for core system functionality. However, consider:
- Implementing a validation mechanism to ensure these skills are always available and functional.
- Allowing for dynamic priority assignment based on user behavior or system state.
- Documenting the criteria for designating a skill as high priority.
These enhancements would improve system security, performance, and user experience.
159-163: Enhance STT reliability and offline capabilities.
The primary STT module is set to "ovos-stt-plugin-server", which relies on an external server. Consider:
- Implementing a mechanism to seamlessly switch between online and offline STT based on network availability.
- Caching frequently used speech-to-text conversions for improved offline performance.
While having a fallback STT module (ovos-stt-plugin-vosk) is good practice, consider:
- Implementing error handling and logging for STT failures.
- Adding configuration options for users to customize the STT pipeline based on their needs (e.g., accuracy vs. speed, online vs. offline preference).
These enhancements would improve the system's reliability and flexibility in various network conditions.
184-187: Optimize TTS settings and enhance customization.
The
pulse_ducksetting is set to false. Consider enabling this feature to improve audio clarity during TTS playback by temporarily lowering the volume of other audio sources.To enhance user experience, consider implementing:
- Voice selection options for both primary and fallback TTS engines.
- Speed and pitch adjustment settings for TTS output.
- A mechanism to easily switch between online (higher quality) and offline (lower latency) TTS based on network availability or user preference.
These enhancements would provide users with more control over the voice interface and improve overall system flexibility.
188-193: Enhance websocket security configuration.
The websocket is currently configured to listen on all interfaces (0.0.0.0). While this provides flexibility, it may pose security risks in certain environments. Consider:
- Allowing configuration of specific interfaces or IP ranges for websocket connections.
- Implementing IP whitelisting for added security in controlled environments.
SSL is currently disabled. To protect against potential data interception, especially in untrusted networks, consider:
- Enabling SSL by default and providing clear documentation on how to configure SSL certificates.
- Implementing a secure key exchange mechanism for initial connection setup.
While shared connection is enabled, which can be efficient, ensure that proper authentication and authorization mechanisms are in place to prevent unauthorized access to shared resources.
These enhancements would significantly improve the security posture of the websocket communication.
test/unittests/configuration/mycroft.json (1)
19-33: Consider using a more persistent cache location.The current cache path (
/tmp/mycroft/cache) is in a temporary directory, which might not persist across system reboots. Consider using a more persistent location, such as/var/cache/mycroftor a user-specific directory like~/.cache/mycroft.Other general settings look appropriate for a default configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- ovos_config/recommends/offline_male/fr-fr.conf (0 hunks)
- ovos_config/recommends/offline_male/it-it.conf (0 hunks)
- ovos_config/recommends/offline_male/nl-nl.conf (0 hunks)
- test/unittests/configuration/init.py (1 hunks)
- test/unittests/configuration/mycroft.conf (1 hunks)
- test/unittests/configuration/mycroft.json (1 hunks)
- test/unittests/configuration/mycroft.yml (1 hunks)
- test/unittests/configuration/test_configuration.py (1 hunks)
💤 Files with no reviewable changes (3)
- ovos_config/recommends/offline_male/fr-fr.conf
- ovos_config/recommends/offline_male/it-it.conf
- ovos_config/recommends/offline_male/nl-nl.conf
✅ Files skipped from review due to trivial changes (1)
- test/unittests/configuration/init.py
🧰 Additional context used
🔇 Additional comments (10)
test/unittests/configuration/mycroft.conf (4)
1-9: LGTM: Well-structured configuration fileThe overall structure of the configuration file is well-organized and follows proper JSON formatting. The use of nested objects for related settings (e.g., "tts" and its sub-settings) is appropriate and enhances readability.
2-2: Correct language code for Brazilian PortugueseThe language setting "pt-br" is the correct ISO 639-1 code for Brazilian Portuguese. This aligns with the PR objective of configuring Mycroft for Brazilian Portuguese.
1-9: Consider additional configuration settings for comprehensive testingWhile this configuration file provides basic language and TTS settings, which may be sufficient for specific unit tests, consider whether additional configuration settings are needed for more comprehensive testing. Typical Mycroft configurations often include:
- Speech-to-Text (STT) settings
- Skill configurations
- System-specific settings
Adding these elements, if relevant to the tests, could provide a more realistic test environment and potentially uncover edge cases or integration issues.
To check if other test configuration files include these additional settings, you can run:
#!/bin/bash # Search for other test configuration files and their contents fd -e conf -e json -e yaml . test | xargs -I {} sh -c 'echo "File: {}"; cat {} | jq -C . 2>/dev/null || cat {}; echo "\n"'This will help determine if the minimal configuration in this file is intentional or if it should be expanded to match other test configurations.
3-8: Verify optimal eSpeak voice for Brazilian PortugueseThe TTS configuration using eSpeak is appropriate, as it supports multiple languages including Portuguese. The "f1" voice setting likely refers to a female voice, which is a common convention in eSpeak.
However, it's worth verifying if "f1" is the optimal voice for Brazilian Portuguese in eSpeak. There might be more specific or higher-quality voices available for this language.
To verify the available voices for Brazilian Portuguese in eSpeak, you can run the following command:
This will list all available Portuguese voices. Check if there's a more suitable voice specifically for Brazilian Portuguese (pt-br).
test/unittests/configuration/test_configuration.py (3)
13-31: LGTM! Well-structured test for RemoteConf.The
test_remotemethod effectively tests theRemoteConfclass by:
- Mocking external dependencies (
RemoteConfigManagerandis_paired).- Setting up a mock API with predefined configuration data.
- Verifying that methods are called as expected (
downloadandis_paired).- Checking that configuration values are correctly retrieved.
This comprehensive approach ensures that the
RemoteConfclass behaves correctly under controlled conditions.
83-90: LGTM! Robust test for YAML configuration loading.The
test_yaml_config_loadmethod effectively verifies the correct loading of YAML configurations by:
- Ensuring that loaded data structures are dictionaries and not OrderedDicts.
- Checking both the main configuration and nested structures.
- Verifying that the configuration can be serialized to JSON and deserialized back without loss of information.
This comprehensive approach ensures that YAML configurations are handled correctly and maintain their structure throughout processing.
1-90: Overall, excellent test suite for configuration management.This test file provides comprehensive coverage for the
LocalConfandRemoteConfclasses, including:
- Remote configuration handling and pairing.
- Local configuration loading, merging, and storing.
- File format compatibility between YAML and JSON.
- Correct loading and structure preservation of YAML configurations.
The use of mocking is appropriate and effective in isolating the system under test. The suggestions provided are minor and aim to further enhance an already robust test suite.
Great job on creating a thorough and well-structured set of tests!
test/unittests/configuration/mycroft.json (3)
2-18: Audio configuration looks good.The audio configuration provides a flexible setup with multiple backends (OCP, simple, and vlc) and sets OCP as the default. This approach allows for fallback options and adaptability to different audio requirements.
1-234: Overall assessment of the configuration file.This configuration file provides a comprehensive setup for a voice assistant system. It covers crucial aspects such as audio handling, wake word detection, STT, TTS, skills management, and system settings. However, there are several areas that could be improved:
- Security: Consider the suggestions for the websocket configuration and wake word upload settings.
- Flexibility: Make certain hardcoded values (like STT server URL) more configurable.
- Performance: Review wake word detection thresholds and expand TTS configuration options.
- Privacy: Implement user consent mechanisms for data collection features.
- Persistence: Use a more permanent location for the cache directory.
While the current configuration is functional, implementing these suggestions would result in a more robust, secure, and user-friendly system.
To ensure the configuration is valid JSON, you can run:
40-58: Review wake word detection thresholds.The wake word detection thresholds for both "hey mycroft" (1e-90) and "wake up" (1e-20) seem extremely low. Such low thresholds might lead to frequent false positive activations. Consider adjusting these values based on real-world testing to find a balance between sensitivity and accuracy.
The use of multiple wake words and different engines (Precise for "hey mycroft" and PocketSphinx for "wake up") provides good flexibility for users.
To help verify the impact of these thresholds, you could run the following script:
|
not sure why tests passed in core but not in this repo, any ideas @NeonDaniel ? |
Maybe a difference in configuration patching? Or just the FileWatcher not triggering an update? I've had inconsistent results with changes to config files triggering a reload.. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation