Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Update for TestTokenQueries.py and TestWebserver.py (#975)
* Added dynamic conda environment name to assertion log messages Using str.format() to update the log message to be matched with the current conda environment name. Then using encode() to convert it back to a byte string. ------ Python byte strings https://stackoverflow.com/questions/6224052/what-is-the-difference-between-a-string-and-a-byte-string https://www.digitalocean.com/community/tutorials/python-string-encode-decode https://stackoverflow.com/questions/67680296/syntaxerror-f-string-expression-part-cannot-include-a-backslash Found encode() / decode() usage in bottle.py e-mission-server/emission/net/api/bottle.py utf-8 not needed as it is default. * Changed TestWebserver.py to work with both config files and ENV variables ------- A. TestWebserver.py Modified the code to have both approaches to read from config file as well as environment variables. The config files approach is based on the previous code logic that was used in this file to copy webserver.conf.sample as webserver.conf. b20cc36 The difference is I'm taking a backup of the webserver.conf file if present. Then writing the test values to this file before running tests. In teardown(), the original webserver.conf is restored from the backup file. ----------- B. cfc_webapp.py WEBSERVER_NOT_FOUND_REDIRECT was missing in the config var_map passed to get_config. It was being read using config.get in the variable not_found_redirect. Hence added it. * Logging CONDA_DEFAULT_ENV to investigate failed tests Tests failed in CI/CD. On testing locally it was able to fetch the `emissiontest` name. But in the CI/CD, it is defaulting to use the `emission` environment name. I did test locally using the steps in the manual install workflow file to use the activate_tests.sh and setup_tests.sh scripts. Now testing locally by running the docker-compose.tests.yml which is used in the test-with-docker workflow. * Corrected 1st log message for local emission environment The tests failed since despite using ENV vars, it wasn’t picking up value. This was because ENV vars are being deleted in setUp(). Added CONDA_DEFAULT_ENV to the skipped env vars. However, this approach of fetching ENV vars programmatically in the assert statement isn’t correct. This is because the code in get_config() itself uses ENV vars to set the ret_val which is the exact value as being fetched in setUp(). Thus the values will always match. ——— Then I thought of adding another log message for the `emission` environment name. While testing this I found that the 1st message isn’t being used in the normal situations when testing locally. This message is observed when the conf/storage/db.conf file is used instead of the sample file. But in normal operation, the server repo doesn’t have any conf files to begin with. Additionally, we now set the DB_HOST as db and WEB_SERVER_HOST as 0.0.0.0 in the Dockerfile by default. Hence the assert log message should also contain `Config file not found…` message like the other two messages. It should also have DB_HOST set to Db like the 3rd message. * Correcting import for restoreOriginalEnvVars * Added fourth message for running tests in local docker container Details in this PR comment: #975 (comment) * Revert "Changed TestWebserver.py to work with both config files and ENV variables" This reverts commit 1dc4275. * Added WEBSERVER_NOT_FOUND_REDIRECT to config map WEBSERVER_NOT_FOUND_REDIRECT was missing in the config var_map passed to get_config. It was being read using config.get in the variable not_found_redirect. * Simplified tests with two log messages for four test environments The PATH values vary based on a user's local system and could have additional values which could fail the test in a local environment run without docker containers. Why do we even need the PATH, PYTHONPATH values in the assertion log messages. This is because we are printing the config in get_database.py which contains environment variables or config file values. Now, the question is why to print the entire set of values if we are only using DB_HOST, DB_RESULT_LIMIT? I am now printing only these values in the get_database.py. This has two benefits: - Allows to eliminate variable environment variables like PATH that might vary in local testing environments for every user. - Since PATH is no longer printed, helps avoid issue of 'apple security' values. - Helps reduce the number of log messages the 4 testing environments: local without docker, local with docker, CI/CD without docker, CI/CD with docker. The two docker environments have the same log message; similarly for the two non-docker environments. Thus we only need two log messages in the assertion tests. * Logging retrieved db config values before using default values With reference to this commit: #975 (comment) So we have two key things to solve: - Unnecessary variables like PATH, that may vary for each user to be removed (helps avoid "apple security" values). - Need to see ENV vars values before being modified by default values in config.get() lines. Two approaches: 1. Delete PATH, PYTHONPATH from the retrieved config. - This wouldn't help much since we still need to handle the scenario when db config environment values haven't been set. 2. Using another config dictionary before modifying with default values - If db config related keys exist, set it to the value from the retrieved config. - Else if key does not exist in retrieved config, which means config file not present AND environment variables not set, then set it to None. Note: - This is not the same as changing the retrieved config value as the original dictionary is still intact. - Additionally, in the case where config file not present AND environment variables not set, there are no values to before modifying with default values. So manually setting to None should be okay. * ✅ Unify the expected messages through redaction Principled fix for the issue outlined in #975 The fundamental issue is that we check the output to validate that the script is behaving as expected. However, the script output can vary in different environments. We had originally handled this by having multiple expected output strings. But as we expand the scope of what we print out, the set of expected output strings expanded. Some of the strings also included paths, so had the username embedded, so would be hard to enumerate in the test. Those logs are also not critical to this test, which is focused on the response to various arguments passed in to the script. So we fix this by simply removing the parts that vary across environments, replacing them by `REDACTED`. As a bonus, this also allows us to collapse the previous two strings into one, and supports all three testing environments: (i) locally on laptop, (ii) locally in docker, and (iii) docker CI/CD (although (ii) is not really supported since it is not tested extensively). Testing done: ``` $ ./e-mission-py.bash emission/tests/storageTests/TestTokenQueries.py ---------------------------------------------------------------------- Ran 21 tests in 22.862s OK ``` * Added missing import for 'os' module Was causing an exception leading to "push not configured" message everytime irrespective of whether config files present or env variables configured. Code with error: ``` try: logging.info(f"Push configured for app {push_config.get('PUSH_SERVER_AUTH_TOKEN')} using platform {os.getenv('PUSH_PROVIDER')} with token {os.getenv('PUSH_SERVER_AUTH_TOKEN')[:10]}... of length {len(os.getenv('PUSH_SERVER_AUTH_TOKEN'))}") except: logging.warning("push service not configured, push notifications not supported") ``` --------- Co-authored-by: Mahadik, Mukul Chandrakant <[email protected]> Co-authored-by: Shankari <[email protected]>
- Loading branch information
3 people
authored
Sep 10, 2024
1 parent
7f450a5
commit 9fe1b3e