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: Missing test coverage #2474

Merged
merged 32 commits into from
Feb 29, 2024
Merged

fix: Missing test coverage #2474

merged 32 commits into from
Feb 29, 2024

Conversation

PROFeNoM
Copy link
Contributor

@PROFeNoM PROFeNoM commented Jan 17, 2024

Description

Oops, I guess I forgot about them in the initial PR #2464

Two Issues:

  1. There were no coverage reports for Integrations because of compiled files (_generated*.php). These, not being the source files, were not counted as coverage. To fix this issue, un-compiled files are used.
  2. Solving the latter issue resolved the problems related to test_integrations_*, but there was still no web test coverage. This is because a new Process is spawned in the web tests. Therefore, phpunit+xdebug doesn't have any visibility over what's happening, and no coverage is captured (despite it being where the valuable information lies). To fix this issue, a script (save_code_coverage.php) will be prepended to the queried file to collect the code coverage manually. These generated code coverage files will be saved in reports/cov, where they can be merged to a clover file using make merge_coverage_reports.

Notes:

  • PHPUnit was removed from some composer.json, as it would sometimes conflict with the script (which uses phpunit's capabilities)
  • tests/Appsec/Mock.php was modified to prevent its element from being loaded twice. This happened because of the introduction of save_code_coverage.php, which requires vendor/autoload.php
  • The log "Backing up auto_prepend_file '%s'" is moved to a debug-type one, as it would 1. make the tests fail and 2. shouldn't be a warning.
  • Some tests that were too flaky/failing under coverage mode were skipped. They are anyway tested when running in non-coverage mode.
  • Coverage tests (integrations & web) are only tested on 7.4 & 8.2, to save time.
  • The tracer-integrations and tracer-php-api flags have been merged into one flag, tracer-php. This makes it more comprehensive.

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@PROFeNoM PROFeNoM self-assigned this Jan 17, 2024
@PROFeNoM PROFeNoM marked this pull request as ready for review January 17, 2024 10:10
@PROFeNoM PROFeNoM requested a review from a team as a code owner January 17, 2024 10:10
@PROFeNoM PROFeNoM added the ci label Jan 17, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Merging #2474 (c46a638) into master (552b0ef) will decrease coverage by 11.67%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2474       +/-   ##
=============================================
- Coverage     76.55%   64.88%   -11.67%     
- Complexity      267     2574     +2307     
=============================================
  Files           138      214       +76     
  Lines         17457    23044     +5587     
  Branches        976        0      -976     
=============================================
+ Hits          13364    14953     +1589     
- Misses         3573     8091     +4518     
+ Partials        520        0      -520     
Flag Coverage Δ
appsec-extension ?
tracer-extension 78.69% <100.00%> (ø)
tracer-integrations ?
tracer-php 47.82% <ø> (?)

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

Files Coverage Δ
ext/request_hooks.c 93.24% <100.00%> (ø)

... and 132 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 552b0ef...c46a638. Read the comment docs.

@PROFeNoM PROFeNoM marked this pull request as draft January 17, 2024 11:03
@PROFeNoM PROFeNoM changed the title feat: Add unit tests coverage (Legacy PHP API) fix: Missing test coverage Jan 17, 2024
@PROFeNoM PROFeNoM added the 🐛 bug Something isn't working label Jan 17, 2024
@pr-commenter
Copy link

pr-commenter bot commented Jan 18, 2024

Benchmarks

Benchmark execution time: 2024-02-29 11:33:48

Comparing candidate commit fdbf9ae in PR branch alex/feat/unit-tests-coverage with baseline commit 552b0ef in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 182 metrics, 0 unstable metrics.

@PROFeNoM PROFeNoM marked this pull request as ready for review January 19, 2024 10:58
@@ -1316,7 +1318,7 @@ test_scenario_%:
$(Q) $(COMPOSER_TESTS) scenario $*

merge_coverage_reports:
$(PHPCOV) merge --clover reports/coverage.xml reports/cov
php -d memory_limit=-1 $(PHPCOV) merge --clover reports/coverage.xml reports/cov
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason behind memory_limit=-1 here is the number of .cov files being processed. I'm talking about 100s of coverage files being merged (1 per request + 1 per test suite).

Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looking good, let's merge it before the branches diverge :-)

@PROFeNoM PROFeNoM force-pushed the alex/feat/unit-tests-coverage branch from 6c7bf49 to 69b3df4 Compare February 13, 2024 12:28
@PROFeNoM
Copy link
Contributor Author

Just did a final rebase onto master to be sure everything is good 👍 Will merge afterwards

@PROFeNoM PROFeNoM force-pushed the alex/feat/unit-tests-coverage branch from 7e4d258 to 3655a4f Compare February 29, 2024 08:01
Copy link
Contributor

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

If you need to update snapshots, please refer to CONTRIBUTING.md

@PROFeNoM PROFeNoM merged commit 58c5db3 into master Feb 29, 2024
606 checks passed
@PROFeNoM PROFeNoM deleted the alex/feat/unit-tests-coverage branch February 29, 2024 14:00
@github-actions github-actions bot added this to the 0.99.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants