Skip to content

Add option to ignore Lua destructor test with warning #16040

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

twrightsman
Copy link
Contributor

Some uncommon architectures (e.g. armel) fail the destructor test but users may still want to use them without disabling all unit tests.

This is a potential workaround for #16031 in situations where we want to ensure the other unit tests still pass.

To do

This PR is Ready for Review.

How to test

Add -DIGNORE_LUA_DESTRUCTOR_TEST_FAILURE=TRUE to cmake options and test on one of the known failing architectures: armel, armhf, loong64, or mips64el.

Some uncommon architectures (e.g. armel) fail the destructor test but
users may still want to use them without disabling all unit tests.
@sfan5
Copy link
Collaborator

sfan5 commented Apr 18, 2025

I get the idea, but if you as a packager are conditionally enabling this option it should be equally easy to just patch the test out as needed.

@twrightsman
Copy link
Contributor Author

should be equally easy

In theory, yes 🙂 Though Debian tooling favors the single patch for all architectures.

That being said, this patch really borders on being Debian-specific and I would completely understand if you decide this is something not appropriate to be included in Luanti upstream. A more general test skipping mechanism would probably be better.

@appgurueu
Copy link
Contributor

appgurueu commented Apr 19, 2025

A more general test skipping mechanism would probably be better.

I agree. We use Catch2 for new tests which could provide this. The problem currently just is that many tests don't use Catch2 yet. I have a patch which converts the tests in question to use Catch2: https://github.com/appgurueu/minetest/tree/test/lua

The problem is just that if we abuse --test-module '~[lua_destructors]', it will only run the Catch2 tests (because all non-Catch2 tests obviously aren't called ~[lua_destructors]).
For an intermediate solution, we could potentially just add another command line option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants