Skip to content

fix: test assertion macros reporting false passes - #87

Merged
tecnolgd merged 2 commits into
tecnolgd:mainfrom
vovosta:patch-1
Oct 22, 2026
Merged

tecnolgd merged 2 commits into
tecnolgd:mainfrom
vovosta:patch-1

Conversation

@vovosta

@vovosta vovosta commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

I made a few changes to address issue #83.

I haven't been able to run the tests locally yet, so these changes have not been verified locally. Feedback and testing are welcome!

What's Changed

  • Fixed the ASSERT_THROW macro by adding a check to ensure that the expected exception is actually thrown.
  • Modified the EXPECT_* macros so that the test continues running even when an expectation fails. Added a counter to track failed expectations and ensured that these failures are reflected in the final test results.

Related Issue

Checklist

  • Code builds successfully without compiler warnings or errors
  • Added or updated unit/regression tests for the changes
  • All tests pass successfully locally
  • Documentation updated (if applicable)

I made a few changes to address issue tecnolgd#83.

For the first part, I fixed the `ASSERT_THROW` macro by adding a check to ensure that the expected exception is actually thrown.

For the second part, I modified the `EXPECT_*` macros so that the test continues running even when an expectation fails. I also added a counter to track the number of failed expectations and made sure that these failures are reflected in the final test results.

I haven't been able to run the tests locally yet, so these changes have not been verified locally. Feedback and testing are welcome!
@vovosta

vovosta commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

If there's any issues do not hesitate to tell me !

@tecnolgd

tecnolgd commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Hey @vovosta, thanks for the fix!

Before we get this merged, could you please update your pull request title to something more descriptive (for example: Fix test assertion macros reporting false passes)? It helps keep our commit history clean. Check the relevant boxes in the checklist as per the changes.
Also, since you mentioned you couldn't run tests locally, make sure you have executed make test to confirm your changes to test_framework.hpp passed successfully! Once those are green, your PR is good to go.

@vovosta

vovosta commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Hey, i'll change the name right now.
I just have an issue : i don't have my main pc available right now, and i don't have enough storage on the pc i have right now. I don't know if it would be possible if someone else tries the code ?
I'm really sorry to ask that but i really don't have any choices for now.

Sorry to bother you,

@vovosta vovosta changed the title Fixes #83 Fix: test assertion macros reporting false passes Oct 22, 2026
@tecnolgd

tecnolgd commented Oct 22, 2026 •

Copy link
Copy Markdown
Owner

Hey, i'll change the name right now. I just have an issue : i don't have my main pc available right now, and i don't have enough storage on the pc i have right now. I don't know if it would be possible if someone else tries the code ? I'm really sorry to ask that but i really don't have any choices for now.

Sorry to bother you,

No worries at all! I totally understand.

The CI is completely green, but I just need some time to perform local tests to double-check everything before merging.
Thanks for updating the title(did some minor edits myself which I forgot to mention earlier) and for the solid work on the macro fix.

@tecnolgd tecnolgd changed the title Fix: test assertion macros reporting false passes fix: test assertion macros reporting false passes Oct 22, 2026
@vovosta

vovosta commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Hey, i think i found a way to compile it locally.
I resolved an issue for an other repo and they told me how to test it on github using the action tab.
I try it and i'll comeback to you soon.

@vovosta

vovosta commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

After all, i'll let you do it because i'm having some issues rn, sorry.
Also, thanks for doing it !!!!

@tecnolgd

Copy link
Copy Markdown
Owner

I verified the changes locally. Everything LGTM. Thanks again!

@tecnolgd
tecnolgd merged commit 7381cdd into tecnolgd:main Oct 22, 2026
5 checks passed
@vovosta

vovosta commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks so much ! I'm so happy to contribute to the project !

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.

[BUG]: Test assertion macros can report failing checks as passed

2 participants