Skip to content

Conversation

Blurazzle
Copy link
Contributor

Description:
This PR introduces the ability to run unit tests using SQLite, which can be helpful when an IBM i system is not available. It also enables running the unit tests on pull requests across multiple PHP versions using GitHub Actions. Additionally, it addresses some compatibility issues with ODBC connections in PHP 8.4.

Key Changes:

  • Improved ODBC connection handling to support \Odbc\Connection and fix compatibility issues in PHP 8.4.
  • Updated unit tests to dynamically determine the DSN based on configuration and added SQLite support through the mockDb2UsingSqlite option for improved flexibility and maintainability.

I understand that these changes might not have been requested, so if they're not of interest or not aligned with the project's goals, I completely understand. Thank you for considering these updates! 😊

- Added support for running unit tests using SQLite by introducing `mockDb2UsingSqlite` config option.
- Refactored DSN handling with a `buildDsn` method for better maintainability.
- Fixed issues with ODBC connection handling, improving compatibility with PHP 8.4.
- Updated tests to dynamically determine the DSN based on the configuration.

This makes it easier to run tests without an IBM i system and allows for automated testing across multiple PHP versions.
@NattyNarwhal
Copy link
Collaborator

I like the idea of this, though I'm not sure how much you can actually test with a mocked database, since you can't call XMLSERVICE. I think the param building stuff can be tested without a DB connection?

@Blurazzle
Copy link
Contributor Author

You're right that XMLSERVICE can't be tested this way. However, using sqlite3 allowed me to:

A more idiomatic approach would probably involve using Mockery, though I'm not sure how complex that would be. I tried this solution but didn’t fully grasp it, which is why I fell back to SQLite.

I believe there’s room for improvement in code and maintainability, but I’m not sure what aligns with the project's goals and scope. For example, when I opened #212 to resolve PHP 8.4 issues, my specific problems were solved. However, there is still code with implicitly nullable types. Also, fix #212 introduced a regression that you later resolved in f21f7c7.

Tools like PHP-CS-Fixer and PHPStan could help with these issues. I have experience with them, and I think one or both could improve code quality. Adding unit tests is also an option, but in my experience, it is often more involved.

@Blurazzle Blurazzle changed the title Enable SQLite for Unit Testing & Improve ODBC Compatibility Enable SQLite for Unit Testing & Improve ODBC Compatibility in PHP 8.4 Mar 17, 2025
@NattyNarwhal
Copy link
Collaborator

Could you split the ODBC changes from the SQLite mocking into a separate PR?

I have a suspicion a different approach for the mocking part could be a dummy backend (as we have for i.e. the DB/SSH/HTTP ones); this might be a way to avoid putting mocking code in the DB backend.

@NattyNarwhal
Copy link
Collaborator

NattyNarwhal commented Mar 17, 2025

FWIW, if you do need access to an IBM i system with PHP (to test things that might be harder to run on a local system), let us know. We also need to figure out a CI story for the Toolkit in general as you've found out.

@Blurazzle
Copy link
Contributor Author

Blurazzle commented Mar 19, 2025

@NattyNarwhal It might be useful to differentiate between integration tests and unit tests and to skip integration tests on systems that are not i5 or do not have an configuration file.

I think I can invest some time in building unit tests for important parts of the library that are currently not covered. I am mainly considering some methods of the XMLWrapper class and possibly the Toolkit class as well. Would this be helpful and what areas would be important to cover?

@NattyNarwhal
Copy link
Collaborator

Sorry for the late response. The unit tests would be good to have. I think the way the Toolkit is factored is a bit awkward; I think I'd still prefer a null/debug driver, but this seems isolated to the unit tests so it shouldn't be too bad to merge this.

@NattyNarwhal NattyNarwhal merged commit 86d3d3f into zendtech:master Mar 31, 2025
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.

2 participants