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.

Sign up for GitHub

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

Update for TestTokenQueries.py and TestWebserver.py #975

Merged
merged 12 commits into from
Sep 10, 2024
Merged

Update for TestTokenQueries.py and TestWebserver.py #975

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d0bc264
Added dynamic conda environment name to assertion log messages
Aug 21, 2024
1dc4275
Changed TestWebserver.py to work with both config files and ENV varia…
Aug 22, 2024
7ac46aa
Logging CONDA_DEFAULT_ENV to investigate failed tests
Aug 22, 2024
32681cf
Corrected 1st log message for local emission environment
Aug 22, 2024
769346c
Correcting import for restoreOriginalEnvVars
Aug 23, 2024
39885a3
Added fourth message for running tests in local docker container
Aug 23, 2024
9ab6358
Revert "Changed TestWebserver.py to work with both config files and E…
Aug 23, 2024
1a13a49
Added WEBSERVER_NOT_FOUND_REDIRECT to config map
Aug 23, 2024
0efbdc8
Simplified tests with two log messages for four test environments
Aug 26, 2024
9bba6c8
Logging retrieved db config values before using default values
Aug 29, 2024
b2aefce
✅ Unify the expected messages through redaction
shankari Sep 1, 2024
6368598
Added missing import for 'os' module
Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion emission/net/api/cfc_webapp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@

config = ecbc.get_config('conf/net/api/webserver.conf',
{"WEBSERVER_PORT": "server.port", "WEBSERVER_TIMEOUT": "server.timeout",
"WEBSERVER_AUTH": "server.auth", "WEBSERVER_AGGREGATE_CALL_AUTH": "server.aggregate_call_auth"})
"WEBSERVER_AUTH": "server.auth", "WEBSERVER_AGGREGATE_CALL_AUTH": "server.aggregate_call_auth",
"WEBSERVER_NOT_FOUND_REDIRECT": "paths.404_redirect"})
server_port = config.get("WEBSERVER_PORT", 8080)
socket_timeout = config.get("WEBSERVER_TIMEOUT", 3600)
auth_method = config.get("WEBSERVER_AUTH", "skip")
Expand Down
46 changes: 45 additions & 1 deletion emission/tests/netTests/TestWebserver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import uuid
import logging
import time
import shutil

# Our imports
import emission.tests.common as etc
Expand All @@ -23,6 +24,40 @@

class TestWebserver(unittest.TestCase):
def setUp(self):
# Backwards_Compatibility method to read from config files
self.webserver_conf_path = "conf/net/api/webserver.conf"
Copy link
Contributor

Choose a reason for hiding this comment

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

when will we ever have a webserver.conf file that we need to back up in a test environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when will we ever have a webserver.conf file that we need to back up in a test environment?

After clarification regarding the testing environments, I do not think we'll have this situation and won't need changes in TestWebserver.py

Note also that we currently support testing in three environments:
In a linux CI/CD environment, using docker compose
In a linux CI/CD environment, running from a local install
In a Mac environment, running from a local install

The keywords here are "test environment".
I had assumed that we might have conf files present in test environment in case someone wanted to check whether values were being read correctly from the config file.
If the test was run in the presence of webserver.conf file then it was failing and hence I thought it was something to fix.

But if it is true that we will not have any conf (non-sample) files in testing environments, then the changes related to TestWebserver.py are not required.

Copy link
Contributor

@shankari shankari Aug 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think we need to consider the use case of an arbitrary config file in automated tests. The whole point of the automated tests is that they run in a known, reproducible environment and generate known, reproducible results. This is particularly true for backwards compatible code, which will be removed soon.

Having said all that, more tests are not bad. As I said "This is not incorrect, but I am not sure why it is required.". If you can make the case for having this test, we can include it and remove it when we remove the backwards compat code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can skip the changes in the test since I wasn't adding any new tests. I had just added code that ensured we read from the config file (non-sample) if present. But it's clear to me now that this isn't necessary in a known testing environment.

self.backup_file_path = "conf/net/api/webserver.conf.bak"

if os.path.exists(self.webserver_conf_path):
shutil.copy2(self.webserver_conf_path, self.backup_file_path)
shutil.copyfile(f"{self.webserver_conf_path}.sample", self.webserver_conf_path)

with open(self.webserver_conf_path, "w") as fd:
fd.write(
json.dumps(
{
"paths": {
"static_path": "webapp/www",
"python_path": "main",
"log_base_dir": ".",
"log_file": "debug.log",
"404_redirect": "http://somewhere.else",
},
"server": {
"host": "0.0.0.0",
"port": "8080",
"timeout": "3600",
"auth": "skip",
"aggregate_call_auth": "no_auth",
},
}
)
)
logging.debug("Finished setting up %s" % self.webserver_conf_path)
with open(self.webserver_conf_path) as fd:
logging.debug("Current values are %s" % json.load(fd))

# New method that uses environment variables only
self.originalWebserverEnvVars = {}
self.testModifiedEnvVars = {
'WEBSERVER_NOT_FOUND_REDIRECT' : "http://somewhere.else"
Expand All @@ -39,6 +74,15 @@ def setUp(self):
logging.debug("Current modified values are = %s" % self.testModifiedEnvVars)

def tearDown(self):
if os.path.exists(self.webserver_conf_path):
os.remove(self.webserver_conf_path)

if os.path.exists(self.backup_file_path):
shutil.move(self.backup_file_path, self.webserver_conf_path)
with open(self.webserver_conf_path, 'rb') as fd:
restored_config = json.load(fd)
logging.debug("Restored file contents: %s" % restored_config)

logging.debug("Deleting test webserver environment variables")
etc.restoreOriginalEnvVars(self.originalWebserverEnvVars,
self.testModifiedEnvVars)
Expand All @@ -54,7 +98,7 @@ def test404Redirect(self):

enacw.error404("")
self.assertEqual(response.status_code, 301)
self.assertEqual(response.get_header("Location"), "http://somewhere.else")
# self.assertEqual(response.get_header("Location"), "http://somewhere.else")

from unittest import mock
@mock.patch.dict(os.environ, {"STUDY_CONFIG":"nrel-commute"}, clear=True)
Expand Down
16 changes: 8 additions & 8 deletions emission/tests/storageTests/TestTokenQueries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,24 +168,24 @@ def test_run_script_show(self):
esdt.insert({'token':'y'})
esdt.insert({'token':'z'})
sp = subprocess.run(["python3", "bin/auth/insert_tokens.py", "--show"], capture_output=True)
# The first message is displayed when we run tests locally
# The first message is displayed when we run tests locally with the emission environment; we are now setting DB_HOST to `db` by default
Copy link
Contributor

Choose a reason for hiding this comment

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

this is true only in dockerized environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I see the difference now in the testing environment I was using.

# The second is displayed when we run in the CI/CD, but with the local install
# The third is displayed when we run in the docker CI since the `DB_HOST` is set to `db`
self.assertIn(sp.stdout,
[b'Retrieved config {\'DB_HOST\': \'localhost\', \'DB_RESULT_LIMIT\': 250000}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'PATH\': \'/home/runner/miniconda-23.5.2/envs/emissiontest/bin:/home/runner/miniconda-23.5.2/condabin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/runner/.dotnet/tools\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emissiontest/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nx\ny\nz\n'
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emission/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'PATH\': \'/home/runner/miniconda-23.5.2/envs/emissiontest/bin:/home/runner/miniconda-23.5.2/condabin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/runner/.dotnet/tools\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emissiontest/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nx\ny\nz\n'
])

def test_run_script_empty(self):
sp = subprocess.run(["python3", "bin/auth/insert_tokens.py"], capture_output=True)
# The first message is displayed when we run tests locally
# The first message is displayed when we run tests locally with the emission environment; we are now setting DB_HOST to `db` by default
# The second is displayed when we run in the CI/CD, but with the local install
# The third is displayed when we run in the docker CI since the `DB_HOST` is set to `db`
self.assertIn(sp.stdout,
[b'Retrieved config {\'DB_HOST\': \'localhost\', \'DB_RESULT_LIMIT\': 250000}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'PATH\': \'/home/runner/miniconda-23.5.2/envs/emissiontest/bin:/home/runner/miniconda-23.5.2/condabin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/runner/.dotnet/tools\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emissiontest/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nPlease provide the script with an argument. Use the "--help" option for more details\n'
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emission/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'PATH\': \'/home/runner/miniconda-23.5.2/envs/emissiontest/bin:/home/runner/miniconda-23.5.2/condabin:/snap/bin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/runner/.dotnet/tools\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config {\'PYTHONPATH\': \'.\', \'DB_HOST\': \'db\', \'PATH\': \'/root/miniconda-23.5.2/envs/emissiontest/bin:/root/miniconda-23.5.2/condabin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin\', \'LC_CTYPE\': \'C.UTF-8\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nPlease provide the script with an argument. Use the "--help" option for more details\n'
])

#test that no two options can be used together
Expand Down
Loading