Skip to content

tests: robustify system symbol overrides #17865

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.

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

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jul 8, 2025

Follow-up to 3bb5e58 #17827


memdebug.h should be included last, but for tests this was
not done like that since the introduction of bundle builds. It
hasn't caused an issue in practice. Let's de-duplicate memdebug.h
includes to make this exception more visible.

In case it causes problem in the future, the way to fix is to undef
(via curl_mem_undef.h merged in #17827) redefinitions before
including per-source headers that include a system header that's
sensitive to those redefinitions. This happens rarely in test code.

@vszakats vszakats marked this pull request as draft July 8, 2025 17:12
@testclutch

This comment was marked as resolved.

@vszakats vszakats force-pushed the t-libtest-memdebugh branch 2 times, most recently from c66e0ac to 7ddbf80 Compare July 9, 2025 18:36
@vszakats vszakats force-pushed the t-libtest-memdebugh branch 2 times, most recently from 7d74ea5 to 3a5a9f6 Compare July 27, 2025 21:04
@vszakats vszakats added the feature-window A merge of this requires an open feature window label Jul 28, 2025
@vszakats vszakats force-pushed the t-libtest-memdebugh branch 2 times, most recently from 29ad8a3 to 7e1577b Compare July 28, 2025 16:46
@vszakats vszakats force-pushed the t-libtest-memdebugh branch from 7e1577b to e301c93 Compare July 28, 2025 17:11
@vszakats vszakats removed feature-window A merge of this requires an open feature window tidy-up labels Jul 31, 2025
@vszakats vszakats changed the title tests: drop duplicate memdebug.h includes tests: tidy up system symbol overrides Jul 31, 2025
@vszakats
Copy link
Member Author

vszakats commented Jul 31, 2025

Before the override refactor (3bb5e58), overrides were never reset in tests,.
which didn't cause issues with current code, but was fragile
(ref: #18066).

After the refactor, tests do reset overrides in an accidental way:
If an individual test source includes an internal curl header for
the first time
, which includes curl_setup.h, it resets the overrides.

In such case a per-source memdebug.h include is necessary.
In other cases it's redundant.

Dedicing which is happening for an individual source is non-trivial.

So we'd want to keep including memdebug.h in each test source
just in case.

But that still doesn't solve the accidental reset behavior. To make
sure it's done for each source file, perhaps it should be done from
first.h, and possibly ask curl_setup.h to not reset when compiling
tests.

But, but, if we disable this feature in curl_setup.h anyway,
perhaps it's better to reset/override manually when necessary?
For libtests this'd save code, because it's rarely necessary. For
units, it might code, because it's mostly necessary.

@vszakats vszakats changed the title tests: tidy up system symbol overrides tests: robustify system symbol overrides Jul 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants