-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: failing AppAsyncPaymentHandlerTest after JWT refactoring #13602
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
Conversation
|
📸 OpenAPI SnapshotTrack changes in your API specifications. Project:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #13602 +/- ##
==========================================
- Coverage 52.00% 52.00% -0.01%
==========================================
Files 3337 3337
Lines 98629 98633 +4
==========================================
- Hits 51295 51292 -3
- Misses 47334 47341 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| */ | ||
| public function testPayFinalizeWithUnsignedResponseOldStruct(): void | ||
| { | ||
| Feature::skipTestIfActive('v6.8.0.0', $this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep old tests until new releases everywhere else we are using instead
#[DisabledFeatures(['v6.8.0.0'])]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's the variant for Unit tests, as there, everything is enabled by default and we can disable at runtime. The feature flags of integration tests are not manipulatable this way (e.g. because of container differences with and without FF, therefore the pipeline (should) execute them once with and once without feature flags and we skip where necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then those do nothing 🙈 afaik the attribute is not even used in integration tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check with a branch by removing those annotations in those tests, but from
Line 173 in 1bd1c2b
| <bootstrap class="Shopware\Core\Test\PHPUnit\Extension\FeatureFlag\FeatureFlagExtension"/> |
| new TestPreparationStartedSubscriber($savedConfig), |
shopware/src/Core/Test/PHPUnit/Extension/FeatureFlag/Subscriber/TestPreparationStartedSubscriber.php
Line 41 in 2727c29
| $disabledFeatures = array_merge( |
I see nothing that technically prevents those annotations to be consumed in integration tests.
What did I miss? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 maybe I am wrong 🙈 but afaik, they did not work properly, when I tried them 🤔

1. Why is this change necessary?
After JWT refactoring of the payment token, where somehow the AppAsyncPaymentHandlerTest never failed, it does now.
2. What does this change do, exactly?
Needs to be refactored to JWT refactoring.
3. Describe each step to reproduce the issue or behaviour.
4. Please link to the relevant issues (if any).
5. Checklist
RELEASE_INFO-6.<major>.mdunder “Upcoming” for informational changes, including the consequences of the change and how it affects external developers.UPGRADEsection inUPGRADE-6.<next-major>.mdfor breaking changes (what/why/impact/how to adapt).