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

Better exception message for when default value is an enum #1380

Merged
merged 4 commits into from
Dec 7, 2023
Merged

Better exception message for when default value is an enum #1380

merged 4 commits into from
Dec 7, 2023

Conversation

forrest79
Copy link
Contributor

This solves an issue #1376.

]));
$classInfo = $reflector->reflectClass('Bat');

$this->expectException(UnableToCompileNode::class);
Copy link
Member

Choose a reason for hiding this comment

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

We should check at least part of the exception message in this test: it seems like the mutation test reports no behavioral change (removes throw and is happy with that), which indicates that an assertion is to be added here

@Ocramius Ocramius changed the base branch from 6.18.x to 6.19.x November 24, 2023 18:33
@Ocramius Ocramius added this to the 6.19.0 milestone Nov 24, 2023
@forrest79
Copy link
Contributor Author

Sorry for a delay, I was working remotely for a couple of days. I added a message check. You can look at it.

@forrest79 forrest79 removed their assignment Dec 4, 2023
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

CS issues fixable with some minimal assertion improvements (by removing try-catch)

test/unit/NodeCompiler/CompileNodeToValueTest.php Outdated Show resolved Hide resolved
@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2023

I think the failures are unrelated to your patch, and instead depend on shivammathur/setup-php#799

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @forrest79!

I will re-run tests and merge once the upstream issue with the CI pipeline is resolved, then releasing 💪

@Ocramius
Copy link
Member

Ocramius commented Dec 6, 2023

Needs shivammathur/php-ubuntu#3

@Ocramius Ocramius self-assigned this Dec 7, 2023
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Excellent, thanks @forrest79!

@Ocramius Ocramius merged commit 92e9cbd into Roave:6.19.x Dec 7, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants