Skip to content

Consolidate test_multidynamic_link and test_multiply_defined_libsymbols. NFC#26986

Merged
sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:test_multiply_defined_libsymbols
May 21, 2026
Merged

Consolidate test_multidynamic_link and test_multiply_defined_libsymbols. NFC#26986
sbc100 merged 1 commit into
emscripten-core:mainfrom
sbc100:test_multiply_defined_libsymbols

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented May 21, 2026

Since these tests were largely redundant, consolidate them into a single parameterized test. Specifically, test_multiply_defined_libsymbols was more comprehensive but test_multidynamic_link tested versioned suffixes and full path linking. test_multiply_defined_libsymbols now covers those cases.

@sbc100 sbc100 changed the title Consolidate test_multidynamic_link and test_multiply_defined_libsymbols Consolidate test_multidynamic_link and test_multiply_defined_libsymbols. NFC May 21, 2026
@sbc100 sbc100 requested a review from kripken May 21, 2026 15:16
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented May 21, 2026

I suspected these two tests were redundant, so I had gemini analyze them and got these results.

I've confirmed that test_multidynamic_link and test_multiply_defined_libsymbols were largely redundant. Both were testing that Emscripten's "fake dylib" model (where .so files are treated
as object files) correctly handles multiple inclusions of the same library without causing multiple definition errors.

Comparison

  • test_multiply_defined_libsymbols: A more comprehensive "diamond dependency" test (main -> liba2 -> libA, main -> libb2 -> libA, and main -> libA directly). It also explicitly verifies
    that emcc issues a warning when ignoring a dynamic library during intermediate object file generation.
  • test_multidynamic_link: A simpler "chain dependency" test (main -> libother -> libfile). Its only unique contributions were testing versioned suffixes (e.g., .so.1.2.3) and linking by
    full path rather than -l.

Action Taken
I have consolidated the tests by:

  1. Enhancing test_multiply_defined_libsymbols: Added parameterization to cover versioned suffixes and full path linking, ensuring no loss of coverage.
  2. Removing test_multidynamic_link: Since its core logic and unique edge cases are now covered by the more robust test_multiply_defined_libsymbols, it has been removed.

@sbc100 sbc100 requested a review from dschuff May 21, 2026 16:41
@sbc100 sbc100 enabled auto-merge (squash) May 21, 2026 16:41
Comment thread test/test_other.py Outdated
…mbols`

Since these tests were largely redundant, consolidate them into a single
parameterized test. Specifically, test_multiply_defined_libsymbols was
more comprehensive but test_multidynamic_link tested versioned suffixes
and full path linking. test_multiply_defined_libsymbols now covers those
cases.
@sbc100 sbc100 force-pushed the test_multiply_defined_libsymbols branch from 0581179 to e953387 Compare May 21, 2026 17:02
@sbc100 sbc100 merged commit cb5cf56 into emscripten-core:main May 21, 2026
30 checks passed
@sbc100 sbc100 deleted the test_multiply_defined_libsymbols branch May 21, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants