8000 [Mailer] Changed the region in the tests to the (default) `eu-west-1` by cafferata · Pull Request #52958 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Mailer] Changed the region in the tests to the (default) eu-west-1 #52958

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.

Sign up for GitHub

By clicking &ldquo 8000 ;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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Mailer] Changed the region in the tests to the (default) eu-west-1 #52958

wants to merge 1 commit into from

Conversation

cafferata
Copy link
Contributor
Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT 
./phpunit src/Symfony/Component/Mailer

Before

PHPUnit output 
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing /symfony/symfony/src/Symfony/Component/Mailer
F...F...F.F...F...F............................................  63 / 495 ( 12%)
............................................................... 126 / 495 ( 25%)
............................................................... 189 / 495 ( 38%)
............................................................... 252 / 495 ( 50%)
............................................................... 315 / 495 ( 63%)
............................................................... 378 / 495 ( 76%)
.......................................SSS...SS.............S.. 441 / 495 ( 89%)
..................SS..................................          495 / 495 (100%)

Time: 00:02.082, Memory: 24.00 MB

There were 6 failures:

1) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testToString with data set #0 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesApiAsyncAwsTransport Object (...), 'ses+api://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+api://ACCESS_KEY@us-east-1'
+'ses+api://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:34

2) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testToString with data set #4 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesApiAsyncAwsTransport Object (...), 'ses+api://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+api://ACCESS_KEY@us-east-1'
+'ses+api://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:34

3) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testSend
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://email.us-east-1.amazonaws.com/v2/email/outbound-emails'
+'https://email.eu-west-1.amazonaws.com/v2/email/outbound-emails'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:79
/symfony/symfony/src/Symfony/Component/HttpClient/MockHttpClient.php:70
/symfony/symfony/vendor/async-aws/core/src/AbstractApi.php:161
/symfony/symfony/vendor/async-aws/ses/src/SesClient.php:67
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesHttpAsyncAwsTransport.php:55
/symfony/symfony/src/Symfony/Component/Mailer/Transport/AbstractTransport.php:69
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:119

4) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testToString with data set #0 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesHttpAsyncAwsTransport Object (...), 'ses+https://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+https://ACCESS_KEY@us-east-1'
+'ses+https://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:34

5) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testToString with data set #4 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesHttpAsyncAwsTransport Object (...), 'ses+https://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+https://ACCESS_KEY@us-east-1'
+'ses+https://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:34

6) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testSend
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://email.us-east-1.amazonaws.com/v2/email/outbound-emails'
+'https://email.eu-west-1.amazonaws.com/v2/email/outbound-emails'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:79
/symfony/symfony/src/Symfony/Component/HttpClient/MockHttpClient.php:70
/symfony/symfony/vendor/async-aws/core/src/AbstractApi.php:161
/symfony/symfony/vendor/async-aws/ses/src/SesClient.php:67
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesHttpAsyncAwsTransport.php:55
/symfony/symfony/src/Symfony/Component/Mailer/Transport/AbstractTransport.php:69
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:112

FAILURES!
Tests: 495, Assertions: 923, Failures: 6, Skipped: 8.

After

PHPUnit output 
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing /symfony/symfony/src/Symfony/Component/Mailer
...............................................................  63 / 495 ( 12%)
............................................................... 126 / 495 ( 25%)
............................................................... 189 / 495 ( 38%)
............................................................... 252 / 495 ( 50%)
............................................................... 315 / 495 ( 63%)
............................................................... 378 / 495 ( 76%)
.......................................SSS...SS.............S.. 441 / 495 ( 89%)
..................SS..................................          495 / 495 (100%)

Time: 00:02.387, Memory: 24.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 495, Assertions: 943, Skipped: 8.

@carsonbot carsonbot added this to the 7.1 milestone Dec 9, 2023
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@cafferata cafferata changed the title [Mailer] Changed the region to the eu-west-1 in the tests [Mailer] Changed the region in the tests to the (default) eu-west-1 Dec 9, 2023
@xabbuh
Copy link
Member
xabbuh commented Dec 9, 2023

Thank you for the PR. Though this doesn't look like the right solution to me (see the failing tests). Can you check if you have locally set one of the environment variables AWS_REGION or AWS_DEFAULT_REGION which could cause the tests to fail locally? If that's the case we should IMO harden our test suite and also add an additional test case that exposes these env vars.

@OskarStark
Copy link
Contributor

Please double check the target branch, my feelings are, that this fix must be applied on a lower branch. Thanks

@cafferata
Copy link
Contributor Author

@xabbuh Thanks for you quick response! I saw these failing on the Symfony #hackday introduction with running the PHPUnit test suite. I've lookup the local variables up front but there are no configurations at all. It's an clean clone of the repository.

Closing this one because the pipeline in main is green. Possibly a local problem. 🤷🏼‍♂️

src/Symfony/Component/Mailer/Bridge/Amazon                                  1s
  + /home/runner/work/symfony/symfony/phpunit --exclude-group tty,benchmark,intl-data,integration src/Symfony/Component/Mailer/Bridge/Amazon
  PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

  Testing /home/runner/work/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon
  .................................................                 49 / 49 (100%)

  Time: 00:00.048, Memory: 12.00 MB

  OK (49 tests, 76 assertions)

  OK src/Symfony/Component/Mailer/Bridge/Amazon

https://github.com/symfony/symfony/actions/runs/7142696464/job/19452537711

@cafferata cafferata closed this Dec 9, 2023
@xabbuh
Copy link
Member
xabbuh commented Dec 9, 2023

@cafferata Neither echo $AWS_REGION nor echo $AWS_DEFAULT_REGION output anything?

@cafferata
Copy link
Contributor Author
cafferata commented Dec 9, 2023 •
edited
Loading

@cafferata Neither echo $AWS_REGION nor echo $AWS_DEFAULT_REGION output anything?

@ 8000 xabbuh that's correct. Any suggestion on 'harden the test suite'?

@cafferata
Copy link
Contributor Author

Please double check the target branch, my feelings are, that this fix must be applied on a lower branch. Thanks

Indeed! It's also on lower branches. But for now I've no idea how to go further with 'harden the test suite'.

@xabbuh
Copy link
Member
xabbuh commented Dec 9, 2023

Could you try to debug locally why AWS Async choses the eu-west-1 region in your case? I think that if we find out the root cause for the test failures on your system we should have a better idea on how we can harden the tests.

@cafferata
Copy link
Contributor Author
cafferata commented Dec 9, 2023 •
edited
Loading

Could you try to debug locally why AWS Async choses the eu-west-1 region in your case?

@xabbuh Found it by dumping $this->sesClient->getConfiguration(). 🙈

^ AsyncAws\Core\Configuration^ {#379
  -data: array:11 [
    "accessKeyId" => "ACCESS_KEY"
    "accessKeySecret" => "SECRET_KEY"
    "region" => "eu-west-1"
    "debug" => "false"
    "profile" => "default"
    "sharedCredentialsFile" => "~/.aws/credentials"
    "sharedConfigFile" => "~/.aws/config"
    "endpoint" => "https://%service%.%region%.amazonaws.com"
    "pathStyleEndpoint" => "false"
    "sendChunkedBody" => "false"
    "endpointDiscoveryEnabled" => "false"
  ]
  -userData: array:3 [
    "accessKeyId" => true
    "accessKeySecret" => true
    "region" => true
  ]
}
cat ~/.aws/config
[default]
region = eu-west-1
output = json

@cafferata
Copy link
Contributor Author

Found an (other) way to stop using the (local) AWS shared configuration in the Composer package async-aws/core in the PHPUnit tests. 🥳

This was an great Symfony hackday pair session with @TmEuMail-2020.

@cafferata cafferata mentioned this pull request Dec 9, 2023
Merged
@cafferata
Copy link
Contributor Author

Thanks @xabbuh and @OskarStark see #52965 for the follow-up PR. 👍

fabpot added a commit that referenced this pull request Dec 9, 2023
@fabpot
minor #52965 [Mailer] Stop using the (local) AWS shared configuration…
ff864b1 
… in PHPUnit (cafferata)

This PR was merged into the 5.4 branch.

Discussion
----------

[Mailer] Stop using the (local) AWS shared configuration in PHPUnit

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Replaces #52958

Found an (other) way to stop using the (local) [AWS shared configuration](https://async-aws.com/authentication/credentials-file.html) in the Composer package [async-aws/core](https://github.com/async-aws/core) in the PHPUnit tests. 🥳

_This was an great Symfony hackday pair session with `@TmEuMail`-2020._

Also big shout out to `@xabbuh` and `@OskarStark` for the heads up in the right direction! 👍

```shell
./phpunit src/Symfony/Component/Mailer
```

**Before**

<details>
    <summary>PHPUnit output</summary>

```shell
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing /symfony/symfony/src/Symfony/Component/Mailer
F...F...F.F...F...F............................................  63 / 495 ( 12%)
............................................................... 126 / 495 ( 25%)
............................................................... 189 / 495 ( 38%)
............................................................... 252 / 495 ( 50%)
............................................................... 315 / 495 ( 63%)
............................................................... 378 / 495 ( 76%)
.......................................SSS...SS.............S.. 441 / 495 ( 89%)
..................SS..................................          495 / 495 (100%)

Time: 00:02.082, Memory: 24.00 MB

There were 6 failures:

1) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testToString with data set #0 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesApiAsyncAwsTransport Object (...), 'ses+api://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+api://ACCESS_KEY@us-east-1'
+'ses+api://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:34

2) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testToString with data set #4 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesApiAsyncAwsTransport Object (...), 'ses+api://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+api://ACCESS_KEY@us-east-1'
+'ses+api://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:34

3) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesApiAsyncAwsTransportTest::testSend
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://email.us-east-1.amazonaws.com/v2/email/outbound-emails'
+'https://email.eu-west-1.amazonaws.com/v2/email/outbound-emails'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:79
/symfony/symfony/src/Symfony/Component/HttpClient/MockHttpClient.php:70
/symfony/symfony/vendor/async-aws/core/src/AbstractApi.php:161
/symfony/symfony/vendor/async-aws/ses/src/SesClient.php:67
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesHttpAsyncAwsTransport.php:55
/symfony/symfony/src/Symfony/Component/Mailer/Transport/AbstractTransport.php:69
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesApiAsyncAwsTransportTest.php:119

4) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testToString with data set #0 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesHttpAsyncAwsTransport Object (...), 'ses+https://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+https://ACCESS_KEY@us-east-1'
+'ses+https://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:34

5) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testToString with data set #4 (Symfony\Component\Mailer\Bridge\Amazon\Transport\SesHttpAsyncAwsTransport Object (...), 'ses+https://ACCESS_KEY@us-east-1')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'ses+https://ACCESS_KEY@us-east-1'
+'ses+https://ACCESS_KEY@eu-west-1'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:34

6) Symfony\Component\Mailer\Bridge\Amazon\Tests\Transport\SesHttpAsyncAwsTransportTest::testSend
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://email.us-east-1.amazonaws.com/v2/email/outbound-emails'
+'https://email.eu-west-1.amazonaws.com/v2/email/outbound-emails'

/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:79
/symfony/symfony/src/Symfony/Component/HttpClient/MockHttpClient.php:70
/symfony/symfony/vendor/async-aws/core/src/AbstractApi.php:161
/symfony/symfony/vendor/async-aws/ses/src/SesClient.php:67
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Transport/SesHttpAsyncAwsTransport.php:55
/symfony/symfony/src/Symfony/Component/Mailer/Transport/AbstractTransport.php:69
/symfony/symfony/src/Symfony/Component/Mailer/Bridge/Amazon/Tests/Transport/SesHttpAsyncAwsTransportTest.php:112

FAILURES!
Tests: 495, Assertions: 923, Failures: 6, Skipped: 8.
```

</details>

**After**

<details>
    <summary>PHPUnit output</summary>

```shell
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Testing /symfony/symfony/src/Symfony/Component/Mailer
...............................................................  63 / 495 ( 12%)
............................................................... 126 / 495 ( 25%)
............................................................... 189 / 495 ( 38%)
............................................................... 252 / 495 ( 50%)
............................................................... 315 / 495 ( 63%)
............................................................... 378 / 495 ( 76%)
.......................................SSS...SS.............S.. 441 / 495 ( 89%)
..................SS..................................          495 / 495 (100%)

Time: 00:02.387, Memory: 24.00 MB

OK, but incomplete, skipped, or risky tests!
Tests: 495, Assertions: 943, Skipped: 8.
```

</details>

Commits
-------

b156c33 [Mailer] Stop using the (local) AWS shared configuration in the PHPUnit tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Mailer Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
4 participants
@cafferata @carsonbot @xabbuh @OskarStark
0