testing env support #288
testing env support #288
Conversation
✅ Deploy Preview for robyn canceled.
|
There was a problem hiding this comment.
Hi @Shending-Help ,
The test structure looks great. I just have some suggestions regarding the implementation.
| path = pathlib.Path(__file__).parent | ||
|
|
||
| #create a directory before test and delete it after test | ||
| def test_create_file(): |
There was a problem hiding this comment.
You can use fixtures(https://docs.pytest.org/en/6.2.x/fixture.html) instead of depending on the execution order of tests.
You would write the test like this
@pytest.fixture
def env_file():
dir = path / "test_dir"
env_file = dir / "robyn.env"
env_file.write_text(CONTENT)
yield
env_file.unlink()
def test_parser(env_file):
...
integration_tests/conftest.py
Outdated
| os.environ["ROBYN_PORT"] = os.environ["PORT"] |
There was a problem hiding this comment.
Try setting ROBYN_URL and ROBYN_PORT directly through load_vars()?
There was a problem hiding this comment.
but shouldn't that be left to the user, it won't always be the case that a user will provide a PORT and a URL in robyn.env but even if he did those vars will be loaded automatically and he can just assign them to a variable and use them
There was a problem hiding this comment.
@Shending-Help , actually the way the env file is supposed to behave in my mind is that if the user sets a config then they don't have to worry about altering the code.
There was a problem hiding this comment.
but that wouldn't be the case for other variables such as a token or db credentials so should we just auto invoke the variables that are relevant for robyn and other variables get left for the user to decide what to do with them?
There was a problem hiding this comment.
No no. What I mean is if you set ROBYN_PORT in the test then it will be automatically present in the environment. That way you don't need to set that variable in the fixture.
If that still doesn't make sense then we can hop on a quick call.
There was a problem hiding this comment.
i think i understand what you mean i will get on it and check with you afterwards
There was a problem hiding this comment.
i think this should be it, I changed the content written in robyn.env to include ROBYN_PORT and ROBYN_URL and it worked, i also deleted the fixture i created in conftest.py since it is of no use now.
| @@ -12,7 +12,7 @@ | |||
| #create robyn.env before test and delete it after test | |||
| @pytest.fixture | |||
| def env_file(): | |||
| CONTENT = "PORT=8080" | |||
| CONTENT = "ROBYN_PORT=8080" + "\n" + "ROBYN_URL=127.0.1.1" | |||
There was a problem hiding this comment.
You can use python multiline strings(https://www.w3schools.com/python/gloss_python_multi_line_strings.asp) here.
unit_tests/test_env_pop.py
Outdated
| env_file.write_text(CONTENT) | ||
| yield | ||
| env_file.unlink() | ||
| os.unsetenv("PORT") |
There was a problem hiding this comment.
This should be ROBYN_PORT and ROBYN_URL now
unit_tests/test_env_pop.py
Outdated
| def test_parser(env_file): | ||
| dir = path / "test_dir" | ||
| env_file = dir / "robyn.env" | ||
| assert list(parser(config_path = env_file)) == [['PORT', '8080']] |
unit_tests/test_env_pop.py
Outdated
| dir = path / "test_dir" | ||
| env_file = dir / "robyn.env" | ||
| load_vars(variables = parser(config_path = env_file)) | ||
| assert os.environ['PORT'] == '8080' |
Description
This PR tests the parser function in env_populator.py from the previous PR, and also tests if variables are set correctly
This PR fixes #287