From 3bacc58bbbfdcc9d1fbb367658086d0b8bdff85c Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Thu, 14 Dec 2023 13:37:52 -0800 Subject: [PATCH] Fix unpredictable markdown link test behavior, clarify test Testing the container-based Bodhi development environment I'm working on exposed a subtle behavior issue in this test/code. The tests have a global fixture - `mock_settings_env_vars` - which sets an environment variable, and a setup method - `BaseTestCaseMixin._setup_method` - which updates `bodhi.server.config.config` (which is a mutable singleton) from the file that env var points to (`tests/testing.ini`). Before the first time the setup method is run, when running the tests in a development environment, `bodhi.server.config.config` will contain the values from `/home/vagrant/development.ini`, the configuration intended for the development deployment, and so any code that's run at this point - including code at the global level in modules that get imported before any test case is run, I believe - uses those values, not the ones from testing.ini. You can test this by ensuring the value is different in the two config files, then adding a line `raise ValueError(config['base_address'])` first at the top of `ffmarkdown.py` - where `UPDATE_RE` was defined, before this commit - and running the test, then moving the line into the `extendMarkdown` method - where this commit moves it to - and running the test. When we check the value at the top of the file, it will be the one from `/home/vagrant/development.ini`; when we check the value from inside `extendMarkdown`, it is the value from `testing.ini`. I think this is because code at the global level gets executed immediately on the first import of the module, which happens before any test case has started to run and so before the setup method has changed the config settings; but `extendMarkdown` will only get called *after* the test case has started running and the setup method has been called. To confuse matters, the test case isn't super explicit about what it's really doing, because it just hardcodes the base_address value from testing.ini into the URL. Changing the test case to show that it's using `config['base_address']` makes it clearer, and moving the update regex definition inside `extendMarkdown` ensures the test and the app code agree on what that value actually is. The regex is not used anywhere else. Alternatively we could leave the regex definition at the global level and have the test code read base_address from `base.original_config`, which is a copy of the 'original' config that `base.py` takes before doing anything to it (so probably somebody ran into this before). But I think I like this way. I noticed this issue because I had to change the base_address value in `/home/vagrant/development.ini` in my container-based environment. In the VM-based environment the base_address value in both development.ini and testing.ini is the same, so the issue is hidden. This doesn't yet address the mystery of why the test's expected result changes the URL from using base_address to using http://localhost, of course. I'll look at that next. Signed-off-by: Adam Williamson --- bodhi-server/bodhi/server/ffmarkdown.py | 13 +++++++++---- bodhi-server/tests/views/test_generic.py | 2 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/bodhi-server/bodhi/server/ffmarkdown.py b/bodhi-server/bodhi/server/ffmarkdown.py index 08597245b3..3839166959 100644 --- a/bodhi-server/bodhi/server/ffmarkdown.py +++ b/bodhi-server/bodhi/server/ffmarkdown.py @@ -41,9 +41,6 @@ BUGZILLA_RE = r'([a-zA-Z]+)(#[0-9]{5,})' -UPDATE_RE = (r'(?:(? str: @@ -204,7 +201,15 @@ def extendMarkdown(self, md: markdown.Markdown) -> None: Args: md: An instance of the Markdown class. """ + # it is intentional that update_re is defined here, not in + # global scope at the top of this file. When testing, defining + # it early results in config['base_address'] being read before + # the test setup method has changed the config to use the + # values from testing.ini and the value may not be as expected + update_re = (r'(?:(?

See '