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

feat(v2): batch validation with partial failure #1621

Open
wants to merge 15 commits into
base: v2
Choose a base branch
from

Conversation

jeromevdl
Copy link
Contributor

@jeromevdl jeromevdl commented Apr 5, 2024

Issue #, if available: #1496

Description of changes:

Adding partial failure for validation with SQS and Kinesis. Modification of the ValidationAspect.java to validate each messages of SQS/Kinesis batches and put invalid messages in partial batch failures list of the response. After the handler, we merge with user batch failures.

Checklist

Breaking change checklist

RFC issue #:

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

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

Copy link

github-actions bot commented Apr 5, 2024

💾 Artifacts Size Report

Module Version Size (KB)
powertools-common 2.0.0-SNAPSHOT 9.63
powertools-serialization 2.0.0-SNAPSHOT 17.23
powertools-logging 2.0.0-SNAPSHOT 33.10
powertools-logging-log4j 2.0.0-SNAPSHOT 20.70
powertools-logging-logback 2.0.0-SNAPSHOT 16.92
powertools-tracing 2.0.0-SNAPSHOT 14.00
powertools-metrics 2.0.0-SNAPSHOT 13.78
powertools-parameters 2.0.0-SNAPSHOT 17.46
powertools-validation 2.0.0-SNAPSHOT 21.32
powertools-cloudformation 2.0.0-SNAPSHOT 16.54
powertools-idempotency-core 2.0.0-SNAPSHOT 34.70
powertools-idempotency-dynamodb 2.0.0-SNAPSHOT 12.38
powertools-large-messages 2.0.0-SNAPSHOT 17.52
powertools-batch 2.0.0-SNAPSHOT 21.51
powertools-parameters-ssm 2.0.0-SNAPSHOT 10.75
powertools-parameters-secrets 2.0.0-SNAPSHOT 9.97
powertools-parameters-dynamodb 2.0.0-SNAPSHOT 12.01
powertools-parameters-appconfig 2.0.0-SNAPSHOT 11.51

Copy link

sonarcloud bot commented Apr 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jeromevdl jeromevdl added the v2 Version 2 label Apr 8, 2024
@scottgerring
Copy link
Contributor

Hey @jeromevdl this is a monster. Do you mind when you have a chance doing a quick writeup of the changes at a high level to start from?

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 89.47368% with 12 lines in your changes missing coverage. Please review.

Project coverage is 77.84%. Comparing base (82d4b30) to head (0923826).
Report is 76 commits behind head on v2.

Current head 0923826 differs from pull request most recent head 503170a

Please upload reports for the commit 503170a to get more accurate results.

Files Patch % Lines
...wertools/validation/internal/ValidationAspect.java 89.47% 7 Missing and 5 partials ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##                 v2    #1621       +/-   ##
=============================================
- Coverage     89.79%   77.84%   -11.96%     
- Complexity      406      479       +73     
=============================================
  Files            44       49        +5     
  Lines          1274     1733      +459     
  Branches        165      261       +96     
=============================================
+ Hits           1144     1349      +205     
- Misses           88      295      +207     
- Partials         42       89       +47     

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

@jeromevdl
Copy link
Contributor Author

@scottgerring this one is ready for review

@jeromevdl
Copy link
Contributor Author

I should create one (or 2 ? kinesis/sqs) end-to-end tests for this maybe, wdyt @scottgerring ?

@scottgerring
Copy link
Contributor

I should create one (or 2 ? kinesis/sqs) end-to-end tests for this maybe, wdyt @scottgerring ?

To be honest I feel like it might be better to invest time stabilizing the existing tests. As it stands more tests are just going to lead to more "was that an actual build failure or not", which has already put us in a situation where we don't trust the suite.

Could we get coverage out of integration style tests instead?

@jeromevdl
Copy link
Contributor Author

Could we get coverage out of integration style tests instead?

Coverage is done with Unit Tests.

And E2E tests are quite stable, sometimes they time out but we rarely troubleshoot failed tests...

Copy link

sonarcloud bot commented Jul 2, 2024

@scottgerring
Copy link
Contributor

Coverage is done with Unit Tests.

I mean, you are concerned that something in here isn't being covered. Can you get it covered without relying on e2e tests?

but we rarely troubleshoot failed tests...

this is the problem I am referring to :D if we don't bother looking when they break.

Copy link

sonarcloud bot commented Aug 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants