Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updated test script for better handling of please enter based prompts… #22

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

Mohsinsiddi
Copy link
Collaborator

Description

This PR introduces two main improvements to the test framework:

  1. Enhanced Environment Variable Handling
  • Added dynamic config-based handling for "Please enter" prompts
  • Improved type detection for different input types (API keys, credentials, RPCs)
  • Better handling of Tenderly, Twitter, and other agent-specific inputs
  • Values are now correctly pulled from agent config files
  1. Improved Test Logging Security
  • Added custom logging handler to mask sensitive information
  • Better organization of pexpect spawn configuration
  • Protected sensitive data like passwords, API keys, and credentials in logs
  • More consistent debug vs non-debug mode handling
  1. Mech Service Enhancement
  • Added health check bypass for mech service
  • Fixed the check_service_health function to handle mech service specially:
if "mech" in config_path.lower():
    logger.info("Bypassing health check for mech service")
    return True, {
        'response_time': 0,
        'status_code': 200,
        'error': None,
        'successful_checks': 1,
        'total_checks': 1
    }

Comment on lines 748 to 786
# Security/Authentication inputs
"password": lambda p: "password" in p,
"backup_owner": lambda p: "backup owner" in p,

# API Keys and Keys
"api_key": lambda p: any(x in p for x in [
"api key",
"api_key",
"apikey",
"tenderly api",
"coingecko api",
"gemini api"
]),

# Tenderly specific inputs
"tenderly_info": lambda p: any(x in p for x in [
"tenderly account",
"tenderly project",
"account slug",
"project slug"
]),

# Twitter related inputs
"twitter_creds": lambda p: any(x in p for x in [
"twitter username",
"twitter email",
"twitter password"
]),

# Technical configurations
"rpc": lambda p: any(x in p for x in ["rpc", "eth_newfilter"]),
"address": lambda p: any(x in p for x in ["address", "contract", "safe"]),

# Content/Description inputs
"persona": lambda p: "persona" in p,

# Navigation inputs
"choice": lambda p: "choice" in p,
"enter": lambda p: any(x in p for x in ["press enter", "continue"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it appears to me that these agent specific checks are bloating the whole thing too much. If I'm not wrong then they were added to resolve that ENS issue right? If yes, then they are not needed anymore. We can clean up a lot here, wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I shall remove this, once PR passes the test

@@ -561,7 +570,7 @@ def get_config_files():
logger.info(f"Found config files: {[f.name for f in config_files]}")

# TODO: Support the test for memeooorr and mech
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this comment also

pull_request:
branches:
- main
pull_request: # Removed branches restriction to run on all PRs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the comment, I guess you wanted me to read this comment? If that's the case then just comment it like this one in the PR. Because the comment in the code, doesn't make sense to a 3rd person.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -182,6 +182,17 @@ def check_docker_status(logger: logging.Logger, config_path: str) -> bool:
def check_service_health(logger: logging.Logger, config_path: str) -> tuple[bool, dict]:
"""Enhanced service health check with metrics."""
service_config = get_service_config(config_path)

if "mech" in config_path.lower():
logger.info("Bypassing health check for mech service")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.info("Bypassing health check for mech service")
logger.info("Bypassing health check for mech service because it's not supported")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -595,7 +626,6 @@ def get_base_config() -> dict:
r"Enter your choice": base_config["STAKING_CHOICE"],
r"Please input your backup owner \(leave empty to skip\)\:": base_config["BACKUP_WALLET"], # Escape parentheses
r"Press enter to continue": "\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
r"Press enter to continue": "\n",
r"Press enter to continue": "\n",
r"Please enter .*": lambda output, logger: handle_env_var_prompt(output, logger, config_path)

why not keep this line here and then remove it from all the agent-specific parts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt do it because as its a get_base_config handler, didnt wanted to pass config paths into it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it too

time.sleep(0.5)
# Send input with a small delay
child.write(response + os.linesep)
time.sleep(2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can avoid these delay now right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to test it then! removed it

@OjusWiZard OjusWiZard merged commit 3060b03 into fix/ENS-error Jan 20, 2025
1 check passed
@OjusWiZard OjusWiZard deleted the feat/mech_tests branch January 20, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants