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

Be stricter about what happens in tests #449

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

greg0ire
Copy link
Contributor

@greg0ire greg0ire commented Jul 3, 2023

This ensures we cannot have a PHP notice or warning in the test suite. Also, we display details when that happens, for easier debugging.

@greg0ire
Copy link
Contributor Author

greg0ire commented Jul 3, 2023

Note: there is no failOnError attribute. When using trigger_error('error', E_USER_ERROR), the test suite does not fail, even with this PR. But since in recent versions of PHP, errors are throwable, maybe that explains why this isn't caught? Anyway, what we really want to catch here are actual notices, warnings and errors, not stuff artificially triggered with trigger_error.

@greg0ire greg0ire marked this pull request as ready for review July 3, 2023 20:17
@jaapio
Copy link
Member

jaapio commented Jul 3, 2023

I think it's good to have this in. I didn't see any errors does that mean that the library is already clean?

@greg0ire
Copy link
Contributor Author

greg0ire commented Jul 4, 2023

I think it's good to have this in. I didn't see any errors does that mean that the library is already clean?

It is already clean indeed, but there is an untested use case for which I'm currently getting a warning, I should send a PR to fix it today.

This ensures we cannot have a PHP notice or warning in the test suite.
Also, we display details when that happens, for easier debugging.
Copy link
Contributor

@linawolf linawolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to be merged once the tests are green

@greg0ire
Copy link
Contributor Author

greg0ire commented Jul 4, 2023

there is an untested use case for which I'm currently getting a warning

Here is what I was talking about: #451

@jaapio jaapio enabled auto-merge July 5, 2023 06:16
@jaapio jaapio disabled auto-merge July 5, 2023 06:19
@jaapio jaapio enabled auto-merge July 5, 2023 06:19
@jaapio jaapio disabled auto-merge July 5, 2023 06:20
@jaapio jaapio merged commit 2ea83f4 into phpDocumentor:main Jul 5, 2023
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.

3 participants