Summary of Changes

Hello @wazir-ahmed, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the documentation and framework for Test Anything Protocol (TAP) tests within ProxySQL. It replaces the previous, less detailed guide with a comprehensive version that clearly distinguishes between unit and integration tests, offering detailed instructions, best practices, and concrete examples for each. This update aims to streamline the testing process, improve test maintainability, and make it easier for developers to contribute robust tests to the project.

Highlights

• Documentation Refactoring: The existing TAP test guide (doc/TAP_TESTS_GUIDE.md) has been replaced with a new, more structured guide (doc/tap_test_guide.md) that provides clearer instructions and best practices.
• Dedicated Unit Testing Section: The new documentation explicitly introduces and details unit testing, including its file structure, best practices, and a comprehensive example template using table-driven testing.
• New Unit Test Example: A new unit test file (test/tap/tests/unit-strip_schema_from_query-t.cpp) has been added, demonstrating how to write isolated unit tests for the strip_schema_from_query() function with various test cases.
• Unit Test Compilation Support: A new header file (test/tap/tests/unit_test.h) was introduced to include necessary macros and headers, resolving linker errors and facilitating the compilation of unit tests.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Hi @wazir-ahmed .
The new documentation is a big improvement - much clearer distinction between unit and integration tests, and the example unit test looks solid. This will definitely help other contributors understand our testing approach better.

A couple of small things to address:

1. Typo in unit_test.h: Line 4 has EXCLUDE_TRACKING_VARAIABLES (extra 'A') - should be EXCLUDE_TRACKING_VARIABLES. Easy fix.
2. Missing documentation about test groups: The guide should mention that new TAP tests need to be added to test/tap/groups/groups.json. This is a key step for CI integration. Could you add a brief section about this? Maybe under "Building Tests" or as its own section. It would be helpful to explain what groups are appropriate for different types of tests.
3. About the ODR violation: I know @gemini-code-assist flagged the GloMyLdapAuth definition in the header. While I understand our TAP tests link against libraries that expect this symbol, it is worth noting this pattern could cause confusion if other developers copy it without understanding the context. Maybe add a comment explaining why we're making this exception?

Overall, great work! Once you fix the typo and add the groups.json documentation, this should be ready to go.

Hi @wazir-ahmed,

I wanted to follow up separately about something I've noticed across several PRs - the use of force pushes (git push --force).
While force pushes have their place, they can create some headaches for collaboration:

1. Breaks review context: When you force push, GitHub loses the connection between the old commits and new ones. This means reviewers (including me and @gemini-code-assist) can't see what changed between review rounds - we lose the incremental history and any inline comments become detached from their original context.
2. Causes issues for other developers: If someone else has fetched your branch to test locally or review, a force push creates conflicts for them. They have to manually clean up their local copy, which interrupts their workflow.
3. Loses development history: The journey of how code evolved - which fixes were tried, what approaches were considered - can be valuable for understanding design decisions later. Force pushes erase that narrative.

For most cases, especially during PR review:

• Small fixes: Use git commit --amend followed by regular push (if you're the only one working on the branch)
• Multiple changes: Just add new commits - we can always squash at merge time

Thoughts?

@renecannao Addressed the review comments. PTAL.

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

LGTM

@renecannao Regarding the force push, here are my thoughts.

1. My habits were mostly influenced by working in teams that preferred linear history. So git rebase [-i] and git push --force are just standard operations for me.
2. I'm aware of the downsides of this approach and agree with you on the benefits of not rewriting commit history once it is pushed and made public.
3. Going forward, I will uphold the sanctity of commit chains :)

Thanks 🙏