Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reduce test framework macrosity #14097

Merged
merged 2 commits into from Dec 15, 2023
Merged

Reduce test framework macrosity #14097

merged 2 commits into from Dec 15, 2023

Conversation

numberZero
Copy link
Contributor

Add compact, short information about your PR for easier understanding:

  • Goal of the PR
    • Cleaner code.
  • How does the PR work?
    • Moves code out of macros.
    • Unifies duplicate code.
    • Keeps test failure information in a single object (the exception), rather than spreading it over rawstream writes from different points.
  • Does it resolve any reported issue?
    • No AFAIK
  • Does this relate to a goal in the roadmap?
    • Yes, 2.2 Internal code refactoring.
  • If not a bug fix, why is this PR needed? What usecases does it solve?

To do

This PR is Ready for Review.

How to test

  • Run unit tests
  • Modify a unit test to make it fail, run it

SmallJoker
SmallJoker reviewed Dec 12, 2023
View reviewed changes
src/unittest/test.h Outdated Show resolved Hide resolved
@sfan5
Copy link
Member

sfan5 commented Dec 12, 2023

I am not disagreeing with this PR, but before putting more work into our unit tests we should decide if we want to switch to catch2 in the future.

@sfan5 sfan5 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Dec 12, 2023
sfan5
sfan5 approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

@numberZero
Copy link
Contributor Author

we should decide if we want to switch to catch2

Decide, then. It’s maintainer’s responsibility to make such decisions.

@numberZero
fixup! Reduce test framework macrosity
48286ea 
Move TestBase::runTest into the cpp
SmallJoker
SmallJoker approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@sfan5
Copy link
Member

sfan5 commented Dec 13, 2023

Decide, then. It’s maintainer’s responsibility to make such decisions.

I do not have an opinion at this time.

@sfan5 sfan5 merged commit 64b5918 into minetest:master Dec 15, 2023
13 checks passed
@numberZero
Copy link
Contributor Author

Thanks.

I do not have an opinion at this time.

Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfan5 sfan5 sfan5 approved these changes

@SmallJoker SmallJoker SmallJoker approved these changes

Assignees
No one assigned
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements >= Two approvals ✅ ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@numberZero @sfan5 @SmallJoker