Skip to content

Commit

Permalink
Fix unpredictable markdown link test behavior, clarify test
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
AdamWill committed Dec 15, 2023
1 parent a5c276c commit 3bacc58
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
13 changes: 9 additions & 4 deletions bodhi-server/bodhi/server/ffmarkdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@


BUGZILLA_RE = r'([a-zA-Z]+)(#[0-9]{5,})'
UPDATE_RE = (r'(?:(?<!\S)|('
+ escape(config['base_address'])
+ r'updates/))([A-Z\-]+-\d{4}-[^\W_]{10})(?:(?=[\.,;:])|(?!\S))')


def user_url(name: str) -> str:
Expand Down Expand Up @@ -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'(?:(?<!\S)|('
+ escape(config['base_address'])
+ r'updates/))([A-Z\-]+-\d{4}-[^\W_]{10})(?:(?=[\.,;:])|(?!\S))')
md.inlinePatterns.register(MentionProcessor(MENTION_RE, md), 'mention', 175)
md.inlinePatterns.register(BugzillaProcessor(BUGZILLA_RE, md), 'bugzilla', 175)
md.inlinePatterns.register(UpdateProcessor(UPDATE_RE, md), 'update', 175)
md.inlinePatterns.register(UpdateProcessor(update_re, md), 'update', 175)
md.postprocessors.register(SurroundPostprocessor(md), 'surround', 175)
2 changes: 1 addition & 1 deletion bodhi-server/tests/views/test_generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ def test_markdown_with_update_alias(self):
def test_markdown_with_update_link(self):
"""Update link should be converted to alias."""
res = self.app.get('/markdown', {
'text': 'See https://bodhi-dev.example.com/updates/FEDORA-2019-1a2b3c4d5e.',
'text': f'See {config["base_address"]}updates/FEDORA-2019-1a2b3c4d5e.',
}, status=200)
assert res.json_body['html'] == \
('<div class="markdown"><p>See '
Expand Down

0 comments on commit 3bacc58

Please sign in to comment.