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

test: remove ddtrace and use datadog file upload #9556

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

djanicekpach
Copy link
Contributor

@djanicekpach djanicekpach commented Jun 24, 2024

test: remove ddtrace and use datadog file upload

Ticket

infeng-747

Description

The --ddtrace operation is giving misleading error messages during tests. This removes ddtrace for now and uploads the tests with the junit xml file upload strategy instead.

Test Plan

Ran CI and passed(or it will be fore I merge). Check Datadog and ensure tests still are showing.

results: https://app.datadoghq.com/ci/test-runs?query=test_level%3Atest%20%40ci.job.url%3A%22https%3A%2F%2Fcircleci.com%2Fgh%2Fdetermined-ai%2Fdetermined%2F2700332%22&agg_m=count&agg_m_source=base&agg_t=count&fromUser=false&index=citest&start=1718638563160&end=1719243363160&paused=false

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.80%. Comparing base (86ed69f) to head (fefd259).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9556   +/-   ##
=======================================
  Coverage   49.80%   49.80%           
=======================================
  Files        1247     1247           
  Lines      162241   162241           
  Branches     2888     2888           
=======================================
+ Hits        80804    80805    +1     
+ Misses      81265    81264    -1     
  Partials      172      172           
Flag Coverage Δ
backend 43.89% <ø> (+<0.01%) ⬆️
harness 63.74% <ø> (ø)
web 46.16% <ø> (ø)

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

see 4 files with indirect coverage changes

Copy link

netlify bot commented Jun 24, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit fefd259
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/6679e5f09542b70008ff842e

@djanicekpach djanicekpach force-pushed the djanicek/infeng-747/remove-ddtrace branch from 571d76f to fefd259 Compare June 24, 2024 21:32
Copy link
Member

@dannysauer dannysauer left a comment

Choose a reason for hiding this comment

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

This looks fine, but one note about some inherited syntax weirdness.

@@ -678,7 +675,10 @@ commands:
pytest_status=$?
Copy link
Member

Choose a reason for hiding this comment

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

I know this isn't changing, but the way the quoting works in this command is bothersome, and I suspect borderline unintentional. The double quotes around master-scheme, master-host, master-port, and junit-xml path look like they're intended to contain the parameters, but because they're inside a double-quoted string, they actually terminate the earlier string and leave the parameter unquoted - quoting the arguments and newlines instead. This clearly works as-is, but it might be worth fixing that in a future PR (or in another commit in this one, I dunno).

Copy link
Member

@dannysauer dannysauer Jun 25, 2024

Choose a reason for hiding this comment

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

It's annoying that the actual referenced code spans a collapsed section of the diff, so the comment doesn't show the code. :/ Stupid Github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yikes! good call. I'll see if I can make that a bit better. I have a couple of PRs in this area

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.

None yet

2 participants