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.

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

Add CI and fix/disable broken tests #39

Merged
merged 26 commits into from
Dec 31, 2022
Merged

Conversation

Jon4t4n
Copy link
Contributor

@Jon4t4n Jon4t4n commented Dec 27, 2022

While I was investigating some crashes that we have been observing in production, I noticed the following:

  1. The driver does not work when compiled with --enable-debug.
  2. Tests seem broken, at least with PHP 7.3 and newer.

Also, it would be nice if we could take advantage of GitHub Actions to ensure that fixes work across multiple PHP versions (and firebird versions).

This PR is a first attempt at adding some basic GitHub Actions that build the driver on Linux (release and debug) and runs the test suite. The majority of the tests are now passing, but some of the tests have been disabled. I intend to file issues for the tests that have been disabled and land fixes for them in follow-up patches.

It feels like the tests have been broken for a while, and thus it would be acceptable to disable the tests that did not have an easy fix. If I am wrong and all the tests should pass, please let me know.

Currently, PHP 7.4, 8.0, 8.1, and 8.2 are covered by CI. I don't know what versions we are trying to support. I would be okay if we completely dropped 7.4 and only have 8.x in CI.

If a test includes `interbase.inc` a dummy database will always be
created, even if the test does not need a database.

This patch splits the `interbase.inc` file into three files so that you
have more control over what is included in a test.
@Jon4t4n Jon4t4n mentioned this pull request Dec 27, 2022
@Jon4t4n
Copy link
Contributor Author

Jon4t4n commented Dec 27, 2022

I guess the actions won't run until this is merged. Adding a link to the latest CI run in my fork.
https://github.com/Jon4t4n/php-firebird/actions/runs/3787871209

@MartinKoeditz
Copy link
Collaborator

Looks good. I will check your work for backward probs. If it's ok I will merge it soon.

Thank you very much.

@Jon4t4n
Copy link
Contributor Author

Jon4t4n commented Dec 28, 2022

Created issues that track the tests that get disabled by this PR. I will update comments/text in the code that refers to GH issues later today, so please wait with the merge until that is done.

The test does not seem to work, disabling temporarily and filing an issue.

Error:
```
Warning: main(): invalid statement handle  in .../tests/004.php on line 112
```
The reason I am marking this test as skipped for the time being is that
I am not sure if this is a bug in the driver or if this is just caused
by me running a newer/older firebird version in CI.

Expected error:
```
lock conflict on no wait transaction deadlock
```

Real error:
```
deadlock read conflicts with concurrent update concurrent transaction number is
```
This patch splits `tests/ibase_param_info_001.phpt` into 3 tests so that
we can check for different error messages depending on which PHP version
we are running.
This patch splits `tests/ibase_num_params_001.phpt` into 4 tests so that
we can check for different error messages depending on which PHP version
we are running.
This patch splits `tests/ibase_num_fields_001.phpt` into 4 tests so that
we can check for different error messages depending on which PHP version
we are running.
This patch splits `tests/ibase_free_query_001.phpt` into 2 tests so that
we can check for different error messages depending on which PHP version
we are running.
This patch splits `tests/ibase_drop_db_001.phpt` into 4 tests so that we
can check for different error messages depending on which PHP version we
are running.
This patch splits `tests/ibase_close_001.phpt` into 3 tests so that we
can check for different error messages depending on which PHP version we
are running.
This patch splits `tests/bug46247.phpt` into 4 tests so that we can
check for different error messages depending on which PHP version we are
running.
When running a debug build of PHP, the engine verifies that the info
supplied in the `ZEND_BEGIN_ARG_INFO_EX` macro is correct. The last
parameter of the `ZEND_BEGIN_ARG_INFO_EX` macro is the number of
required arguments.

If we try to invoke `ibase_rollback_ret` with zero arguments, we get the
following error message:

```
Fatal error: Arginfo / zpp mismatch during call of ibase_rollback_ret() in test.php on line 123
```

This patch corrects this by setting the value to zero to match how the
method is implemented.
When running a debug build of PHP, the engine verifies that the info
supplied in the `ZEND_BEGIN_ARG_INFO_EX` macro is correct. The last
parameter of the `ZEND_BEGIN_ARG_INFO_EX` macro is the number of
required arguments.

If we try to invoke `ibase_commit_ret` with zero arguments, we get the
following error message:

```
Fatal error: Arginfo / zpp mismatch during call of ibase_commit_ret() in test.php on line 123
```

This patch corrects this by setting the value to zero to match how the
method is implemented.
When running a debug build of PHP, the engine verifies that the info
supplied in the `ZEND_BEGIN_ARG_INFO_EX` macro is correct. The last
parameter of the `ZEND_BEGIN_ARG_INFO_EX` macro is the number of
required arguments.

If we try to invoke `ibase_rollback` with zero arguments, we get the
following error message:

```
Fatal error: Arginfo / zpp mismatch during call of ibase_rollback() in test.php on line 123
```

This patch corrects this by setting the value to zero to match how the
method is implemented.
When running a debug build of PHP, the engine verifies that the info
supplied in the `ZEND_BEGIN_ARG_INFO_EX` macro is correct. The last
parameter of the `ZEND_BEGIN_ARG_INFO_EX` macro is the number of
required arguments.

If we try to invoke `ibase_commit` with zero arguments, we get the
following error message:

```
Fatal error: Arginfo / zpp mismatch during call of ibase_commit() in test.php on line 123
```

This patch corrects this by setting the value to zero to match how the
method is implemented.
Running `tests/008.phpt` in a debug build reports a memory leak. Marking
as skipped until the memory leak is fixed.
When running the tests in a debug build, I started seeing the following error:
```
Warning: zend_signal: handler was replaced for signal (X) after startup in
    Unknown on line 0
```

At the time of writing, I have no clue what is causing the issue, but I
started looking at the PDO Firebird driver and this issue was fixed
there as well. See:
php/php-src@a9253b0

This patch does the exact same thing.
@Jon4t4n
Copy link
Contributor Author

Jon4t4n commented Dec 28, 2022

I will update comments/text in the code that refers to GH issues later today, so please wait with the merge until that is done.

Done.

@MartinKoeditz
Copy link
Collaborator

Thank you. I'll run some tests over the weekend and publish your work.

@MartinKoeditz MartinKoeditz merged commit 0c4bb8a into FirebirdSQL:master Dec 31, 2022
@Jon4t4n Jon4t4n deleted the ci branch February 18, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants