Skip to content

chore(CI): starlette tests#10483

Merged
christophe-papazian merged 8 commits intomainfrom
christophe-papazian/fix_appsec_starlette_test
Sep 3, 2024
Merged

chore(CI): starlette tests#10483
christophe-papazian merged 8 commits intomainfrom
christophe-papazian/fix_appsec_starlette_test

Conversation

@christophe-papazian
Copy link
Contributor

@christophe-papazian christophe-papazian commented Sep 3, 2024 •
edited
Loading

Fix and enable again starlette tests that were disabled.
Reason of the bug:

  • starlette was fixed in 0.38.3 for an issue that was causing trouble in our appsec instrumentation. Tests were upgraded accordingly

Fix:
Kludex/starlette#2620
Issue:
Kludex/starlette#2516

Also improve the guards in the asgi middleware appsec instrumentation for request body to ensure that we never break.

APPSEC-53221

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2024

CODEOWNERS have been resolved as:

.github/workflows/test_frameworks.yml                                   @DataDog/python-guild @DataDog/apm-core-python
ddtrace/appsec/_handlers.py                                             @DataDog/asm-python

@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Sep 3, 2024 •
edited
Loading

Datadog Report

Branch report: christophe-papazian/fix_appsec_starlette_test
Commit report: d0288d6
Test service: dd-trace-py

✅ 0 Failed, 2688 Passed, 1679 Skipped, 47m 30.08s Total Time

@pr-commenter
Copy link

pr-commenter bot commented Sep 3, 2024 •
edited
Loading

Benchmarks

Benchmark execution time: 2024-09-03 13:49:18

Comparing candidate commit d0288d6 in PR branch christophe-papazian/fix_appsec_starlette_test with baseline commit 4c845ab in branch main.

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

@christophe-papazian christophe-papazian changed the title tentative fix fix(CI): starlette tests Sep 3, 2024
@christophe-papazian christophe-papazian added changelog/no-changelog A changelog entry is not required for this PR. ASM Application Security Monitoring CI labels Sep 3, 2024
@christophe-papazian christophe-papazian changed the title fix(CI): starlette tests chore(CI): starlette tests Sep 3, 2024
@christophe-papazian christophe-papazian added changelog/no-changelog A changelog entry is not required for this PR. and removed changelog/no-changelog A changelog entry is not required for this PR. labels Sep 3, 2024
@christophe-papazian christophe-papazian marked this pull request as ready for review September 3, 2024 12:55
@christophe-papazian christophe-papazian requested review from a team as code owners September 3, 2024 12:55
@christophe-papazian christophe-papazian enabled auto-merge (squash) September 3, 2024 12:58
@romainkomorn-exdatadog
Copy link
Contributor

romainkomorn-exdatadog commented Sep 3, 2024

Do the workflows (eg: .github/workflows/test_frameworks.yml ) need renaming?

image

@christophe-papazian christophe-papazian merged commit 84b335e into main Sep 3, 2024
@christophe-papazian christophe-papazian deleted the christophe-papazian/fix_appsec_starlette_test branch September 3, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juanjux juanjux juanjux approved these changes

@brettlangdon brettlangdon brettlangdon approved these changes

@avara1986 avara1986 avara1986 approved these changes

@gnufede gnufede Awaiting requested review from gnufede gnufede is a code owner automatically assigned from DataDog/python-guild

@emmettbutler emmettbutler Awaiting requested review from emmettbutler emmettbutler is a code owner automatically assigned from DataDog/apm-core-python

+1 more reviewer

@romainkomorn-exdatadog romainkomorn-exdatadog romainkomorn-exdatadog approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR. CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@christophe-papazian @romainkomorn-exdatadog @juanjux @brettlangdon @avara1986