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 tests after Symfony PropertyInfo update #1973

Conversation

mringler
Copy link
Contributor

@mringler mringler commented Jul 11, 2023

With the current Symfony components, one test in the 5-max and and 6-max group fails, see for example this run.

Apparently, the Symfony PropertyInfo has changed internally, and now an assertion fails. A comment in the test acknowledges that it tests Symfony internals:

        $fictionMetadatas = $fictionMetadata->getPropertyMetadata('isbn');

        // 1st is for ValidateTriggerFiction (base)
        // 2nd is for ValidateTriggerBook (base)
        // I'm not sure if this is needed. We should not care about validator internals
        $this->assertCount(2, $fictionMetadatas);

Examining $fictionMetadatas, I can see that both ValidateTriggerFiction and ValidateTriggerBook are present in $fictionMetadatas[0].constraintsByGroup:

Screenshot_2023-07-11_15-42-15

This is also successfully tested in the rest of the test case.

So apparently the PropertyInfo component now groups those constraints in the same PropertyMetadata object instead of using a new object for each. But this does not concern Propel. So we can just remove that assertion. We could also turn it into assertNotEmpty(), but that is tested implicitly in the next assertion.

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: -1.51 ⚠️

Comparison is base (011aaa7) 88.56% compared to head (d2607c2) 87.05%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1973      +/-   ##
============================================
- Coverage     88.56%   87.05%   -1.51%     
  Complexity     8051     8051              
============================================
  Files           243      232      -11     
  Lines         24590    24544      -46     
============================================
- Hits          21777    21367     -410     
- Misses         2813     3177     +364     
Flag Coverage Δ
5-max 87.05% <ø> (-1.51%) ⬇️
7.4 87.05% <ø> (-1.51%) ⬇️
agnostic 67.44% <ø> (+0.14%) ⬆️
mysql ?
pgsql 69.81% <ø> (+0.70%) ⬆️
sqlite ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 35 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@floriankraemer
Copy link

I'll get this merged, it makes sense.

@gechetspr gechetspr merged commit b9dee04 into propelorm:master Sep 4, 2023
37 checks passed
@mringler mringler deleted the tests/change_in_symfony_PropertyInfo_component branch September 29, 2023 22:13
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.

4 participants