propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 393 forks source link

fix tests after Symfony PropertyInfo update #1973

Closed mringler closed 10 months ago

mringler commented 1 year ago

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 commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -1.51 :warning:

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

:exclamation: 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 ```diff @@ 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%)` | :arrow_down: | | 7.4 | `87.05% <ø> (-1.51%)` | :arrow_down: | | agnostic | `67.44% <ø> (+0.14%)` | :arrow_up: | | mysql | `?` | | | pgsql | `69.81% <ø> (+0.70%)` | :arrow_up: | | sqlite | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=propelorm#carryforward-flags-in-the-pull-request-comment) to find out more. [see 35 files with indirect coverage changes](https://app.codecov.io/gh/propelorm/Propel2/pull/1973/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=propelorm)

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

floriankraemer commented 10 months ago

I'll get this merged, it makes sense.