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

chore: add IAST smoke tests for common packages #9469

Merged
merged 39 commits into from
Jun 12, 2024

Conversation

juanjux
Copy link
Collaborator

@juanjux juanjux commented Jun 3, 2024

Description

Adds smoke + small integration tests of the most used modules. Still in progress, will be split into several PRs. Originally by @avara1986

Changes since the previous reverted PR:

  • Run the install/tests inside a virtual environmnent (one for every package, cloned from an initial one to avoid re-installing all deps) to avoid side effects.
  • Small refactor to simplify things.

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • 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

@juanjux juanjux added the ASM Application Security Monitoring label Jun 3, 2024
@juanjux juanjux self-assigned this Jun 3, 2024
@juanjux juanjux requested review from a team as code owners June 3, 2024 19:59
@juanjux juanjux added the changelog/no-changelog A changelog entry is not required for this PR. label Jun 3, 2024
@juanjux juanjux changed the title Appsec 53118 iast integration tests chore: add smoke tests for common packages Jun 3, 2024
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented Jun 3, 2024

Datadog Report

Branch report: APPSEC-53118-iast_integration_tests
Commit report: 9345f50
Test service: dd-trace-py

✅ 0 Failed, 176131 Passed, 1252 Skipped, 11h 21m 50.01s Total duration (7m 16.54s time saved)

Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
@juanjux juanjux marked this pull request as draft June 4, 2024 08:26
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
@juanjux
Copy link
Collaborator Author

juanjux commented Jun 5, 2024

@juanjux Does this PR need to be reviewed? Or are we holding off until you break it up into the subsequent smaller PRs?

This can be already reviewed, follow up PRs adding more smoke tests for more packages will be smaller. If you want to check only the important bits that changed (this is from a previous PR from Alberto that was merged and then pulled back because the packages were not installed on a virtualenv and had some side-effects with other tests), ignoring the auto generated or semi auto generated files, just check inside_env_runner.py and test_packages.py.

@juanjux juanjux changed the title chore: add smoke tests for common packages chore: add IAST smoke tests for common packages Jun 5, 2024
@pr-commenter
Copy link

pr-commenter bot commented Jun 5, 2024

Benchmarks

Benchmark execution time: 2024-06-12 08:34:46

Comparing candidate commit 9345f50 in PR branch APPSEC-53118-iast_integration_tests with baseline commit ec67b2c in branch main.

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

juanjux and others added 7 commits June 10, 2024 09:54
Co-authored-by: Federico Mon <federico.mon@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

riotfile changes lgtm

@juanjux juanjux enabled auto-merge (squash) June 12, 2024 07:46
@juanjux juanjux disabled auto-merge June 12, 2024 08:18
Signed-off-by: Juanjo Alvarez <juanjo.alvarezmartinez@datadoghq.com>
@juanjux juanjux enabled auto-merge (squash) June 12, 2024 08:34
@juanjux juanjux disabled auto-merge June 12, 2024 08:45
@juanjux juanjux merged commit b472fc6 into main Jun 12, 2024
188 checks passed
@juanjux juanjux deleted the APPSEC-53118-iast_integration_tests branch June 12, 2024 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASM Application Security Monitoring changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants