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

Create test for templateManager #1515

Merged

Conversation

eerison
Copy link
Contributor

@eerison eerison commented Aug 3, 2022

Cover templateManager

I am targeting this branch, because in 4.x the page content is not rendered, and I need to understand first how it was working, something was changed or removed in 4.x that I can't identify.

Issue #1497

@eerison
Copy link
Contributor Author

eerison commented Aug 3, 2022

I'm trying to mock the sonata.templating service, but I looks like removed

root@4d1918ef933d:/app# bin/console debug:container sonata.templating

Information for Service "sonata.templating"
===========================================

 ---------------- ------------------------------------------ 
  Option           Value                                     
 ---------------- ------------------------------------------ 
  Service ID       sonata.templating                         
  Class            Sonata\BlockBundle\Templating\TwigEngine  
  Tags             -                                         
  Public           no                                        
  Synthetic        no                                        
  Lazy             no                                        
  Shared           yes                                       
  Abstract         no                                        
  Autowired        no                                        
  Autoconfigured   no                                        
 ---------------- ------------------------------------------ 


 ! [NOTE] The "sonata.templating" service or alias has been removed or inlined when the container was compiled. 

do you have any idea why @VincentLanglet or @Daric971

Edit 1: and It leads to other question, is this make lint-symfony-container working? because I tried to run this locally passing a service that doesn't exist and it doesn't broken

@VincentLanglet
Copy link
Member

VincentLanglet commented Aug 3, 2022

I know nothing about this service :/

@eerison eerison force-pushed the Create_test_for_template_manager branch from e4fe5b3 to d3b6589 Compare August 3, 2022 08:36
@eerison eerison force-pushed the Create_test_for_template_manager branch from d3b6589 to eca8e4c Compare August 5, 2022 06:04
@eerison eerison force-pushed the Create_test_for_template_manager branch 4 times, most recently from 34af942 to a9eae36 Compare August 15, 2022 12:24
@eerison eerison marked this pull request as ready for review August 15, 2022 12:26
@eerison
Copy link
Contributor Author

eerison commented Aug 15, 2022

Finally I could cover breadcrumb behaviour in test, I hope it helps me to fix in 4.x.

@eerison
Copy link
Contributor Author

eerison commented Aug 15, 2022

Could you check this PR @VincentLanglet @jordisala1991 , please :)

@@ -13,12 +13,12 @@ file that was distributed with this source code.
{% block sonata_page_container %}
<div class="container">
<div class="content">
<div class="row page-header">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we put id often ; so I dont like adding some for test, and would prefer something else than

static::assertCount(1, $crawler->filter('#header'));

Not sure what's sonata_page_render_container is doing but you might find somehting to search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in this case I'm testing the page, and I'm checking if there is an element into the page!
I guess it is fine to test elements into the page using "id", don't you?

Copy link
Member

Choose a reason for hiding this comment

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

breadcrumb, header, content, ... are way too generic words. What if someone use js on it ? What if he's already is using those ?

Adding id, it just a risk of BC-break with no benefit for the code. It's not like we're using js on something.
You're adding them because you're writting tests. And you should never have your test pushing you to modify your src code.

You can work on the already existing classes. Adding page-breadcrumb class and so on might be a feature to allow overriding the design for user ; it seems better than ids.

But I don't understand the purpose of your test, checking for div in the page ; Checking for url, 200, etc might be enough.

@jordisala1991 wrote a lot of tests on 4.x branch and never needed to add id, or to find some div.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I want to make sure is if the breadcrumbs are rendered into the page!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

page-breadcrumb

Ok I'll try to use page-breadcrumb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @VincentLanglet could you check again please!

I just let the testing checking breadcrumb only, and I'm following the same way from page-header

@eerison eerison marked this pull request as draft August 15, 2022 12:59
@eerison eerison force-pushed the Create_test_for_template_manager branch from a9eae36 to baced94 Compare August 16, 2022 06:44
@eerison eerison force-pushed the Create_test_for_template_manager branch from baced94 to dc54cb7 Compare August 16, 2022 06:46
@eerison eerison marked this pull request as ready for review August 16, 2022 06:47
@eerison eerison requested a review from VincentLanglet August 16, 2022 06:48
@eerison
Copy link
Contributor Author

eerison commented Aug 17, 2022

Hey @jordisala1991 could you review this PR please. and if it's possible merge into 4.x, tomorrow I would try to investigate the problem related with #1497

Copy link
Member

@jordisala1991 jordisala1991 left a comment

Choose a reason for hiding this comment

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

IMO this test would be much more easy on 4.x with the tests i have already added for render pages.

@VincentLanglet VincentLanglet merged commit 728cc5b into sonata-project:3.x Aug 17, 2022
@VincentLanglet
Copy link
Member

Can you open a PR of the merge of 3.x into 4.x @eerison ? There is a lot of issue with this test when I try to merge

@eerison eerison deleted the Create_test_for_template_manager branch August 17, 2022 20:58
@eerison
Copy link
Contributor Author

eerison commented Aug 17, 2022

Yeah, tomorrow I can do this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants