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(ci): add the Metrics feature to nox tests #4552

Merged

Conversation

leandrodamascena
Copy link
Contributor

Issue number: #4521

Summary

Changes

Adding the Metrics feature to nox tests

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@leandrodamascena leandrodamascena requested a review from a team June 18, 2024 12:56
@boring-cyborg boring-cyborg bot added the tests label Jun 18, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 18, 2024
@github-actions github-actions bot added the internal Maintenance changes label Jun 18, 2024
@leandrodamascena leandrodamascena linked an issue Jun 18, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.43%. Comparing base (e14e768) to head (09c6933).
Report is 604 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4552      +/-   ##
===========================================
+ Coverage    96.38%   96.43%   +0.05%     
===========================================
  Files          214      219       +5     
  Lines        10030    10628     +598     
  Branches      1846     1976     +130     
===========================================
+ Hits          9667    10249     +582     
- Misses         259      267       +8     
- Partials       104      112       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

heitorlessa
heitorlessa previously approved these changes Jun 18, 2024
Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Q: should we only trust Nux test names to know what's required vs optional?

or (a) use a different test subfolder for each feature, (b) add a top-level docstring for those rearranged tests for optional dependency (like you did on noxfile:68

@leandrodamascena
Copy link
Contributor Author

Q: should we only trust Nux test names to know what's required vs optional?

I think that's a good indication of what dependencies this subset of tests should use, no?

or (a) use a different test subfolder for each feature, (b) add a top-level docstring for those rearranged tests for optional dependency (like you did on noxfile:68

Can you elaborate further and give an example of what this would look like?

@heitorlessa
Copy link
Contributor

Of course!

We can reduce cognitive load on maintainers by making this even more explicit with a folder name that suggests the reason behind this separation.

For example.

  • Now: tests/functional/metrics
  • After:
    • tests/functional/metrics/required_dependencies/ (CloudWatch EMF, no dependency really)
    • tests/functional/metrics/datadog
    • tests/functional/metrics/otel

Alternatively, you could bundle /datadog, /otel under dependencies, but the trade-off is that you lose the ability to selectively test that datadog dependency only.

As a byproduct, this also helps us detect regressions that were introduced in one dependency that now affects another unrelated class of tests.

@leandrodamascena
Copy link
Contributor Author

For example.

  • Now: tests/functional/metrics

  • After:

    • tests/functional/metrics/required_dependencies/ (CloudWatch EMF, no dependency really)
    • tests/functional/metrics/datadog
    • tests/functional/metrics/otel

This is much better @heitorlessa! Thanks for always making things simple ❤️

Comment on lines +58 to +59
f"{PREFIX_TESTS_FUNCTIONAL}/logger/required_dependencies/",
f"{PREFIX_TESTS_FUNCTIONAL}/metrics/required_dependencies/",
Copy link
Contributor

Choose a reason for hiding this comment

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

see how beautiful it is now - both in Nox and on folder structure <3 no ambiguity now!

@heitorlessa heitorlessa merged commit 5626c97 into aws-powertools:develop Jun 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Maintenance changes size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: add the Metrics feature to nox tests
2 participants