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: #20002 optimize head request on serializeDataProvider #20007

Conversation

xicond
Copy link
Contributor

@xicond xicond commented Oct 14, 2023

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Fixed issues #20002

@codecov
Copy link

codecov bot commented Oct 14, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (75e5a08) 48.95% compared to head (c1315b9) 48.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20007      +/-   ##
==========================================
+ Coverage   48.95%   48.99%   +0.03%     
==========================================
  Files         445      445              
  Lines       42844    42845       +1     
==========================================
+ Hits        20973    20990      +17     
+ Misses      21871    21855      -16     
Files Coverage Δ
framework/rest/Serializer.php 64.40% <71.42%> (+8.85%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bizley
Copy link
Member

bizley commented Oct 16, 2023

Cheers, could you add the missing test? And a changelog line.

@bizley bizley added this to the 2.0.50 milestone Oct 16, 2023
@xicond
Copy link
Contributor Author

xicond commented Oct 18, 2023

@bizley

I tried my best to add unit test which not covered before, seems codecov still not satisfy and not detect the coverage
Also the test only for previous state, not testing #20002 kind of bug,

to test #20002, I'd need to include Mockery library $arrayDataProviderMocked->shouldReceive('getModels')->never()
But I dont see its usage before, so I think it's not necessary.

@bizley
Copy link
Member

bizley commented Oct 18, 2023

I'm not sure if adding it in the same test is not interfering with it. Try to exclude it to a new one. Also, Mockery is not needed, PHPUnit is enough.

framework/CHANGELOG.md Outdated Show resolved Hide resolved
@samdark samdark requested review from a team October 19, 2023 17:35
@xicond
Copy link
Contributor Author

xicond commented Oct 20, 2023

I separated the test,
last time nodejs test job 17871509197 failed,
now it's success, seems some tests are unstable

@@ -400,6 +402,11 @@ public function dataProviderSerializeDataProvider()
];
}

public function dataProviderHeadSerializeDataProvider()
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary. Just use the same provider method in your test.

@bizley bizley merged commit 04f5944 into yiisoft:master Oct 20, 2023
49 checks passed
@bizley
Copy link
Member

bizley commented Oct 20, 2023

Cheers

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

Successfully merging this pull request may close these issues.

4 participants