Add unit tests for macros.h

[sjasonsmith]

Description

Marlin macros are at the core of almost all functionality. It is imperative that they work as intended, and be protected against accidental regressions.

This is a large amount of test code which was about 90% written by Github Copilot. There may be inconsistencies in style, but due to the large amount to create it wasn't practical to hand-curate every line.

I do not think it is worthwhile for anybody to review every single line of this. It should suffice to recognize that it passes all PR checks and can begin checking for altered behavior on future submissions.

Requirements

N/A

Benefits

Protect against regressions in macros.h.

Configurations

N/A

Related Issues

N/A

[oliof]

Wait, you are saying these functions are critical and should not have any regressions, but the tests are autogenerated and nobody should thoroughly check whether they test for the right thing?

That sounds ... dangerous. ALso, the unclear copyright situation of CoPilot generated code seems to be a risk for Marlin to adopt something that's been mostly autogenerated where the PR author suggests not reviewing the changes.

I am not implying this is an xz-utils situation, but this seems to be ill-thought out.

[The-EG]

@oliof sounds like you are volunteering to go line by line and check all this out. Thanks!

[oliof]

Tests should be reviewed by people who intimately understand the Macros. I am not that person. Yet I can express my concern about the PR submitters idea of adding critical code without thorough review. I should not suffer passive aggressiveness from anyone for that. That said, I did read the code and it _looks_ reasonable, but that's exactly what CoPilot does: It creates things that _look_ reasonable. It should be reviewed by a subject matter expert. Gesendet von Proton Mail für Android

…

-------- Ursprüngliche Nachricht -------- Am 14.04.24 14:53 um Taylor Talkington schrieb :

***@***.***(https://github.com/oliof) sounds like you are volunteering to go line by line and check all this out. Thanks! — Reply to this email directly, [view it on GitHub](#26968 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAJMOI5PDG2LEXXFMRJS5CLY5J33PAVCNFSM6AAAAABGF3TTPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGA2DONZYG4). You are receiving this because you were mentioned.Message ID: ***@***.***>

[The-EG]

@oliof And I believe no one here should suffer accusations of malintent, whether thinly veiled as questionable ignorance or not. The reality is that if this waited for the type of review you speak of it'd never be completed.

Is it a perfect situation? No, but it is progress and I trust sjasonsmith to have made a decision to have an overall positive impact on this project.

[oliof]

I absolutely agree that this PR was made with the best interests of the project in mind. Gesendet von Proton Mail für Android

…

-------- Ursprüngliche Nachricht -------- Am 14.04.24 15:09 um Taylor Talkington schrieb :

***@***.***(https://github.com/oliof) And I believe no one here should suffer accusations of malintent, whether thinly veiled as questionable ignorance or not. The reality is that if this waited for the type of review you speak of it'd never be completed. Is it a perfect situation? No, but it is progress and I trust sjasonsmith to have made a decision to have an overall positive impact on this project. — Reply to this email directly, [view it on GitHub](#26968 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAJMOIZDNBBBLNA4ZP5ACDDY5J5W3AVCNFSM6AAAAABGF3TTPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGA2TMOBYGU). You are receiving this because you were mentioned.Message ID: ***@***.***>

[sjasonsmith]

Wait, you are saying these functions are critical and should not have any regressions, but the tests are autogenerated and nobody should thoroughly check whether they test for the right thing?

@oliof they were created with Github Copilot, but that is far from being auto-generated. They were still created one block of tests at a time and reviewed by myself. Copilot was instructed to adjust them where I saw flaws or lack of coverage, and I hand-edited a few where the macros are just too confusing for Copilot to work with.

I can express my concern about the PR submitters idea of adding critical code without thorough review.

The tests are not themselves critical code, they test critical code. Do they test everything? Nope, but neither do carefully crafted human tests, and these provide more value than the zero tests that were there previously.

You can think of adding unit tests into Marlin as an experiment at this stage...but we have to get tests in place for that experiment to continue. We will never get any meaningful test coverage if every new test spends months under review.

Ideally for new work tests would be added along with the new code, but as of yet we have no guidelines for that within Marlin.

without thorough review

When adding unit tests to existing code, the goal is less about testing proper behavior, and more about locking down existing behavior. These tests serve as a barometer for accidental behavior alteration, even if the new behavior is improvement. In that case, the decision can be made whether the new code has a bug, or the existing tests should be altered.

[oliof]

Thanks for the detailed reply Jason, this approach makes complete sense to me.

…

-------- Ursprüngliche Nachricht -------- Am 14.04.24 18:44 um Jason Smith schrieb :

> Wait, you are saying these functions are critical and should not have any regressions, but the tests are autogenerated and nobody should thoroughly check whether they test for the right thing? ***@***.***(https://github.com/oliof) they were created with Github Copilot, but that is far from being auto-generated. They were still created one block of tests at a time and reviewed by myself. Copilot was instructed to adjust them where I saw flaws or lack of coverage, and I hand-edited a few where the macros are just too confusing for Copilot to work with. > I can express my concern about the PR submitters idea of adding critical code without thorough review. The tests are not themselves critical code, they test critical code. Do they test everything? Nope, but neither do carefully crafted human tests, and these provide more value than the zero tests that were there previously. You can think of adding unit tests into Marlin as an experiment at this stage...but we have to get tests in place for that experiment to continue. We will never get any meaningful test coverage if every new test spends months under review. Ideally for new work tests would be added along with the new code, but as of yet we have no guidelines for that within Marlin. > without thorough review When adding unit tests to existing code, the goal is less about testing proper behavior, and more about locking down existing behavior. These tests serve as a barometer for accidental behavior alteration, even if the new behavior is improvement. In that case, the decision can be made whether the new code has a bug, or the existing tests should be altered. — Reply to this email directly, [view it on GitHub](#26968 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAJMOIYAWUTUNYI724QJD3TY5KW7VAVCNFSM6AAAAABGF3TTPOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJUGEYTOOJQGQ). You are receiving this because you were mentioned.Message ID: ***@***.***>