Skip to content

[TASK] Add Tests for Document::getAllDeclarationBlocks() #956

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 20, 2025
Merged

[TASK] Add Tests for Document::getAllDeclarationBlocks() #956

merged 3 commits into from
Feb 20, 2025

Conversation

@oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Feb 19, 2025

Also remove a misleading and incorrect comment.

Part of #757

@oliverklee oliverklee added the testing PRs/issues adding additional tests only, or primarily testing-focused label Feb 19, 2025
@oliverklee oliverklee requested a review from JakeQZ February 19, 2025 22:21
@oliverklee oliverklee self-assigned this Feb 19, 2025
@coveralls
Copy link

coveralls commented Feb 19, 2025 •
edited
Loading

Coverage Status

coverage: 53.148% (+1.5%) from 51.654%
when pulling 997eaf7 on tests/get-all-declaration-blocks
into f28fc1f on main.

@oliverklee oliverklee force-pushed the tests/get-all-declaration-blocks branch from 1a44e9b to a1accfb Compare February 20, 2025 08:20
JakeQZ
JakeQZ reviewed Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

The tests look fine. But I find them difficult to follow because I can't immediately see how $this->subject has been setUp(). Would it perhaps be clearer for the subject to be set up on a per-test basis? And maybe a separate pre-PR to remove the setUp() method, which I don't think is warranted.

@oliverklee oliverklee force-pushed the tests/get-all-declaration-blocks branch from a1accfb to 245d68a Compare February 20, 2025 10:02
@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 20, 2025

Done: #963

@oliverklee oliverklee marked this pull request as draft February 20, 2025 10:06
@oliverklee oliverklee force-pushed the tests/get-all-declaration-blocks branch from 245d68a to e822151 Compare February 20, 2025 10:25
@oliverklee oliverklee marked this pull request as ready for review February 20, 2025 10:25
@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 20, 2025

Updated an repushed.

@oliverklee oliverklee requested a review from JakeQZ February 20, 2025 10:25
JakeQZ
JakeQZ requested changes Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ 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 a test method name should not include 'also', and that it would be better to provide non-empty strings which are ideally real-world examples for URL and charset. Otherwise fine.

@oliverklee oliverklee force-pushed the tests/get-all-declaration-blocks branch from 9651087 to 7fac016 Compare February 20, 2025 11:11
@oliverklee oliverklee requested a review from JakeQZ February 20, 2025 11:12
JakeQZ
JakeQZ requested changes Feb 20, 2025
View reviewed changes
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

The method name issue does not seem to be resolved.

@oliverklee oliverklee force-pushed the tests/get-all-declaration-blocks branch from 7fac016 to 997eaf7 Compare February 20, 2025 12:31
@oliverklee
Copy link
Collaborator Author

oliverklee commented Feb 20, 2025 •
edited
Loading

The method name issue does not seem to be resolved.

Indeed. My bad. Done.

@oliverklee oliverklee requested a review from JakeQZ February 20, 2025 12:31
@JakeQZ JakeQZ merged commit 9736e26 into main Feb 20, 2025
21 checks passed
@JakeQZ JakeQZ deleted the tests/get-all-declaration-blocks branch February 20, 2025 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakeQZ JakeQZ JakeQZ approved these changes

Assignees

@oliverklee oliverklee

Labels

testing PRs/issues adding additional tests only, or primarily testing-focused

Projects

None yet

Milestone

9.0.0: Drop support for PHP < 7.2 and...

Development

Successfully merging this pull request may close these issues.

4 participants

@oliverklee @coveralls @JakeQZ