[tests] ToC: ordering condition involving title generation.

[jayaddison]

Describe the bug

When running text-builder tests from the suite from commit bf0bec3 using pytest tests/test_builders/test_build_text.py, the test_nonascii_title_line test case fails when test_secnums is evaluated before test_nonascii_title_line.

How to Reproduce

From a checkout of 23eb545 and using pytest==8.1.1:

$ pytest -vv tests/test_builders/test_build_text.py tests/test_builders/test_build_text.py -k 'secnum or nonascii_title'

(

Environment Information

Platform:              linux; (Linux-6.6.15-rt-amd64-x86_64-with-glibc2.37)
Python version:        3.11.8 (main, Feb  7 2024, 21:52:08) [GCC 13.2.0])
Python implementation: CPython
Sphinx version:        7.3.0+/23eb54538
Docutils version:      0.20.1
Jinja2 version:        3.1.3
Pygments version:      2.17.2

Sphinx extensions

N/A

Additional context

Relates to #11285.

[jayaddison]

I'm starting work on this bug myself, partly because I feel that this may be a good way to learn more about toctree caching issues, something that are apparently causing build output nondeterminism during parallelised builds.

The fix doesn't appear to be as straightforward as I had sensed that it might be when I added the good-first-issue label; even so there could be benefits to figuring out the fix in a newcomer-friendly way.

So far I've adjusted the test_nonascii_title_line test case -- temporarily -- into a shape that more minimally replicates the problem:

@with_text_app()
def test_nonascii_title_line_tmp(app):
    app.config.text_secnumber_suffix = ' '
    app.build()
    app.config.text_secnumber_suffix = '. '
    app.build()
    result = (app.outdir / 'nonascii_title.txt').read_text(encoding='utf8')
    expect_underline = '*********'
    result_underline = result.splitlines()[1].strip()
    assert result_underline == expect_underline

$ pytest tests/test_builders/test_build_text.py -k title_line
...
FAILED tests/test_builders/test_build_text.py::test_nonascii_title_line_tmp - AssertionError: assert '********' == '*********'

[jayaddison]

So far I've adjusted the test_nonascii_title_line test case -- temporarily -- into a shape that more minimally replicates the problem:

Adding some comments to that:

@with_text_app()
def test_nonascii_title_line_tmp(app):
    # after removing these two lines, the test begins passing: why?
    app.config.text_secnumber_suffix = ' '
    app.build()
    # the two lines below are the expected / correct test build configuration
    app.config.text_secnumber_suffix = '. '
    app.build()
    result = (app.outdir / 'nonascii_title.txt').read_text(encoding='utf8')
    expect_underline = '*********'
    result_underline = result.splitlines()[1].strip()
    assert result_underline == expect_underline

[jayaddison]

Referring back to this bug's description: requesting that the test_nonascii_title_line test should use a clean/refreshed Sphinx environment -- for example, by adding the freshenv=True build parameter to the pytest marker, or force_all to the app.build call -- avoids the test failure.

@with_text_app(freshenv=True)  # this avoids the test failure
def test_nonascii_title_line_tmp(app):
    app.build()  # an alternative avoidance is to rewrite this line to: app.build(force_all=True) 
    result = (app.outdir / 'nonascii_title.txt').read_text(encoding='utf8')
    expect_underline = '*********'
    result_underline = result.splitlines()[1].strip()
    assert result_underline == expect_underline

Either of the two presented modifications result in:

$ pytest -vv tests/test_builders/test_build_text.py tests/test_builders/test_build_text.py -k 'secnum or nonascii_title'
...
tests/test_builders/test_build_text.py::test_nonascii_title_line PASSED                                                                        [ 25%]
tests/test_builders/test_build_text.py::test_secnums PASSED                                                                                    [ 50%]
tests/test_builders/test_build_text.py::test_nonascii_title_line PASSED                                                                        [ 50%]
tests/test_builders/test_build_text.py::test_secnums PASSED                                                                                    [ 50%]
...

However, I would like to investigate further and determine whether we could adjust Sphinx to allow these build actions -- that appear valid when written in source code -- to function as expected, without needing to adjust the test.

Edit: 20240907: adjust the retest command and output to more-accurately correspond to the repro case in the bug description.

[jayaddison]

However, I would like to investigate further and determine whether we could adjust Sphinx to allow these build actions -- that appear valid when written in source code -- to function as expected, without needing to adjust the test.

As illustrated in the adjusted minimal repro test case -- this problem occurs when a second text build of a project with a toctree takes place with a different text_secnumber_suffix value.

In general, Sphinx is designed to detect modification to certain configuration settings, and should refresh the doctree that is contained in the environment:

sphinx/sphinx/environment/__init__.py

Lines 344 to 350 in 0438178

	# check if a config value was changed that affects how
	# doctrees are read
	for item in config.filter(frozenset({'env'})):
	if self.config[item.name] != item.value:
	self.config_status = CONFIG_CHANGED
	self.config_status_extra = f' ({item.name!r})'
	break

The env string mentioned in that code is used to annotate settings that should indeed cause a build refresh, and text_secnumber_suffix appears to be one of those:

sphinx/sphinx/builders/text.py

Line 93 in 0438178

app.add_config_value('text_secnumber_suffix', '. ', 'env')

However, there seems to be some code that overrides the resulting config_status -- whether to refresh the build or not -- under circumstances that I've yet to determine:

sphinx/sphinx/builders/__init__.py

Lines 480 to 481 in 0438178

	# workaround: marked as okay to call builder.read() twice in same process
	self.env.config_status = CONFIG_OK

Commenting out the config_status adjustment is an application-only change that allows the minimal test case to pass -- but causes a failure in the test_incremental_reading test case, implying that doing so would have undesirable side-effects.

[jayaddison]

The core of the problem appears to be that an in-process env.config modification does not invalidate the cached configuration used during subsequent project builds.

A question that raises is: should config modification be supported between two adjacent(*) in-process builds?

If we do want to support in-process between-build config modification, then we could adjust the application code to attempt to detect those modifications. This could perhaps re-use the same invalidation detection methods as the file-based (pickle) env cache -- or instead it could use a monotonic timestamp.

If we do not want to support in-process between-build config modification, then we could update the tests so that each of them are isolated (independent) of the other's configuration.

(*) In-process config modification during a build itself would seem like a recipe for undefined and nondeterministic results, especially during parallelised builds. So that's out-of-scope -- this comment relates solely to modifications that occur after a build has already completed and before another build may begin.

[jayaddison]

A question that raises is: should config modification be supported between two adjacent(*) in-process builds?

I think it would be much simpler not to support these. The benefit would seem to be fairly marginal for the majority of anticipated use-cases (single documentation projects built within a single self-contained process during each build) -- and there is hidden future complexity (for example: config-inited events would add complexity to handle correctly).

So it feels to me that we should isolate these two test cases, and offer that as a resolution for this bug.