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

Fix assertTemplateName() checking only the first leaf in the view model tree. #87

Merged
merged 8 commits into from
Sep 16, 2024

Conversation

visto9259
Copy link
Contributor

@visto9259 visto9259 commented Sep 13, 2024

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

Fixes #3.

The protected function AbstractControllerTestCase::searchTemplates() will now traverse all view model children until the template is found, returning false if not found in any children.

@Xerkus
Copy link
Member

Xerkus commented Sep 13, 2024

Please use descriptive title. It will be an entry in the release notes.

@visto9259 visto9259 changed the title To fix issue #3 Fix #3 assertTemplateName returns too early Sep 13, 2024
@visto9259 visto9259 changed the title Fix #3 assertTemplateName returns too early assertTemplateName returns too early Sep 13, 2024
gsteel
gsteel previously requested changes Sep 13, 2024
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Code changes LGTM but the location of the test method should be moved AFAICT.

test/PHPUnit/Controller/TemplateNameTest.php Outdated Show resolved Hide resolved
@gsteel gsteel linked an issue Sep 13, 2024 that may be closed by this pull request
@gsteel gsteel added the Bug Something isn't working label Sep 13, 2024
@Xerkus Xerkus changed the title assertTemplateName returns too early Fix assertTemplateName() checking only the first leaf in the view model tree. Sep 13, 2024
@gsteel gsteel added this to the 4.10.2 milestone Sep 13, 2024
…ll children until found. Added test case and supporting assets to test nested views.

Signed-off-by: Eric Richer [email protected] <[email protected]>
…prove test case to test deeper viewer hierarchy

Signed-off-by: Eric Richer [email protected] <[email protected]>
@Xerkus
Copy link
Member

Xerkus commented Sep 14, 2024

Tests for the template assertion seem to be missing for a case where templates are not found, or found for not variant, and assertion exception is thrown. Can you also add those?

Use try catch block instead of expected exception assertion because assertion exceptions are PhpUnit's and might have special handling attached to them.

… when the template is not found and the 'assertNotTemplateName' fails when the template is found

Signed-off-by: Eric Richer [email protected] <[email protected]>
@visto9259
Copy link
Contributor Author

That was counter intuitive. Adding tests to check that PHPUnit fails...

@Xerkus
Copy link
Member

Xerkus commented Sep 16, 2024

Well, we are not checking that phpunit fails. We are checking that assertion succeeds.

Copy link
Member

@Xerkus Xerkus left a comment

Choose a reason for hiding this comment

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

Did you think this was over? 😉

There are some QA changes I want you to address before it is ready.

… the template is not found and the 'assertNotTemplateName' fails when the template is found. to separate test cases.

Signed-off-by: Eric Richer [email protected] <[email protected]>
…ertTemplateName()' and 'assertNotTemplateName()'

Signed-off-by: Eric Richer [email protected] <[email protected]>
…eNameFailsWhenNotFound()' and 'testAssertNotTemplateNameFailsWhenFound()'

Signed-off-by: Eric Richer [email protected] <[email protected]>
@Xerkus
Copy link
Member

Xerkus commented Sep 16, 2024

CI run stuck

@Xerkus Xerkus merged commit cd714a8 into laminas:4.10.x Sep 16, 2024
11 of 12 checks passed
@Xerkus
Copy link
Member

Xerkus commented Sep 16, 2024

@visto9259 thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assertTemplateName returns too early
3 participants