Skip to content

Fix multiline variable in array and with expr not be coverage #955

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

Conversation

kayw-geek
Copy link

@kayw-geek kayw-geek commented Nov 4, 2022

I think the case need be coverage
Fix #948

@sebastianbergmann
Copy link
Owner

CC @Slamdunk @mvorisek

@kayw-geek
Copy link
Author

image

Before, variable not be coverage

@kayw-geek kayw-geek force-pushed the bugfix/return_with_multiline_variable_in_array_with_expr branch from a11c1e5 to b006a62 Compare November 4, 2022 09:56
@mvorisek
Copy link
Contributor

mvorisek commented Nov 4, 2022

please submit this PR into my #949 PR, the analysis is rewritten a lot

@Slamdunk
Copy link
Contributor

Slamdunk commented Nov 4, 2022

@mvorisek you cannot expect a new contributor to know and inspect work that hasn't been merged yet.

We either complete and merge your work and ask @kayw-geek to rebase his PR afterwards, or simply integrate the new test case into #949 tests and fix it by ourselves.

@kayw-geek
Copy link
Author

@mvorisek you cannot expect a new contributor to know and inspect work that hasn't been merged yet.

We either complete and merge your work and ask @kayw-geek to rebase his PR afterwards, or simply integrate the new test case into #949 tests and fix it by ourselves.

It took me half a day to find this problem, and I really want to be a contributor. I will feel honored.

@codecov
Copy link

codecov bot commented Nov 5, 2022

Codecov Report

Merging #955 (1a01b19) into main (2fde509) will increase coverage by 0.15%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##               main     #955      +/-   ##
============================================
+ Coverage     85.06%   85.21%   +0.15%     
- Complexity     1166     1172       +6     
============================================
  Files            69       69              
  Lines          3422     3464      +42     
============================================
+ Hits           2911     2952      +41     
- Misses          511      512       +1     
Impacted Files Coverage Δ
...c/StaticAnalysis/ExecutableLinesFindingVisitor.php 96.87% <93.75%> (-0.69%) ⬇️
src/Report/Html/Renderer.php 99.31% <0.00%> (+0.07%) ⬆️
src/Report/Html/Renderer/File.php 94.90% <0.00%> (+0.12%) ⬆️
src/Report/Html/Renderer/Directory.php 91.07% <0.00%> (+0.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kayw-geek kayw-geek force-pushed the bugfix/return_with_multiline_variable_in_array_with_expr branch 2 times, most recently from 8c91c33 to df0b90f Compare November 9, 2022 05:23
@kayw-geek kayw-geek requested a review from mvorisek November 9, 2022 05:23
@kayw-geek kayw-geek force-pushed the bugfix/return_with_multiline_variable_in_array_with_expr branch 2 times, most recently from b613841 to ca65ac5 Compare November 9, 2022 07:43
@kayw-geek
Copy link
Author

@mvorisek Excuse me, Could you review this PR again. Thanks 🙏

@kayw-geek kayw-geek force-pushed the bugfix/return_with_multiline_variable_in_array_with_expr branch from ca65ac5 to 1a01b19 Compare November 10, 2022 10:22
@kayw-geek kayw-geek requested a review from mvorisek November 10, 2022 10:23
@mvorisek
Copy link
Contributor

see

// line 108 is unstable - variable has no coverage if it holds const expr - https://github.com/sebastianbergmann/php-code-coverage/issues/953
test and explanation in #953 (comment)

to fix the #953 you need to statically analyse the local variable scope for each evaluated node to be able to determine if variable was optimized out at compile time (const expr) or not

@kayw-geek
Copy link
Author

So, your meaning was I need to use the method in the #939 to fix these unstable problems?

@kayw-geek
Copy link
Author

see

// line 108 is unstable - variable has no coverage if it holds const expr - https://github.com/sebastianbergmann/php-code-coverage/issues/953

test and explanation in #953 (comment)
to fix the #953 you need to statically analyse the local variable scope (范围) for each evaluated (评价) node to be able to determine (确定) if variable was optimized out at compile (编译) time (const expr) or not

So, your meaning was I need to use the method in the #939 to fix these unstable problems?

@mvorisek
Copy link
Contributor

you need to evaluate every (sub) AST tree for const expr and track variable scope for each, like done by php optimizer or other static analysers like phpstan

in a meantime, please also review #949, it will help you to understand some edge cases

@Slamdunk
Copy link
Contributor

#964 should have fixed this too, @kayw-geek can you check please?

@kayw-geek kayw-geek closed this Dec 16, 2022
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