[Part 1] Modernise package and tests #91
Conversation
| @@ -27,13 +33,13 @@ jobs: | |||
| ${{ runner.os }}-php- | |||
|
|
|||
| - name: Install dependencies | |||
| run: composer install --prefer-dist --no-progress --no-suggest | |||
| @@ -12,11 +12,23 @@ | |||
| "guzzlehttp/guzzle": "^7.0.1", | |||
| "http-interop/http-factory-guzzle": "^1.0.0" | |||
| }, | |||
| "minimum-stability": "stable", | |||
There was a problem hiding this comment.
Bumped to stable - makes me feel much better.
| "php": ">=7.3", | ||
| "php": "^8.2", |
There was a problem hiding this comment.
Given tests are passing, and we're running this in a PHP 8.2 env (the platform) I think we're fine.
| "Snowcap\\Emarsys\\Tests\\Unit\\": "tests/Unit/Suites/", | ||
| "Snowcap\\Emarsys\\Tests\\Integration\\": "tests/Integration/Suites/" | ||
| } | ||
| }, |
There was a problem hiding this comment.
PHPStorm was unhappy with the namespaces. This should fix them. I moved us to PSR-4, which is why the src directory is now root instead of src/Snowcap/Emarsys/
| @@ -337,6 +340,6 @@ public function testItReturnsEmailResponseSummary(): void | |||
| */ | |||
| private function createExpectedResponse(string $fileName) | |||
| { | |||
| return file_get_contents(__DIR__ . '/TestData/' . $fileName . '.json'); | |||
| return Utils::streamFor(file_get_contents(__DIR__ . '/TestData/' . $fileName . '.json')); | |||
There was a problem hiding this comment.
Guzzle only returns streams now. Rather than just plain old strings.
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
|
|
||
| pull_request: | ||
| types: [ opened, synchronize, reopened ] |
There was a problem hiding this comment.
We were running the CI on push and PR, which didn't make much sense.
| env: # Set the secrets as environment variables | ||
| EMARSYS_API_USERNAME: ${{ secrets.EMARSYS_API_USERNAME }} | ||
| EMARSYS_API_SECRET: ${{ secrets.EMARSYS_API_SECRET }} |
There was a problem hiding this comment.
I'd prefer our integration tests didn't use Emarsys directly (I didn't realise it did until now).
Not on my agenda for today, but will probably change it to a mock server of some sort.
| @@ -1,17 +1,15 @@ | |||
| <?xml version="1.0"?> | |||
| <phpunit | |||
There was a problem hiding this comment.
We're on PHPUnit 9 and it was upset at some of the configs.
| @@ -0,0 +1,10 @@ | |||
| ## Description | |||
There was a problem hiding this comment.
Shamelessly stolen from the monoliths repo.
| @@ -126,7 +127,7 @@ public function testItAddsChoicesMapping(): void | |||
| public function testItThrowsAnExceptionIfFieldDoesNotExist(): void | |||
| { | |||
| $this->expectException(ClientException::class); | |||
| $this->expectErrorMessage('Unrecognized field name "non-existing-field-name"'); | |||
| $this->expectExceptionMessage('Unrecognized field name "non-existing-field-name"'); | |||
There was a problem hiding this comment.
expectErrorMessage is deprecated, and will be removed in PHPUnit 10. For our purposes expectExceptionMessage should work the same.
|
Good improvements, thanks for not letting it gather more debt. |
Description
Every once in awhile, when Dependabot bumps a dependency, I get reminded this repo exists. And what immediately follows is a crippling sense of anxiety that its very out of date, CI is broken, I'm pretty sure this is long since forgotten about (minus me and Sheamus who occasionally get dependency bumps), and tons of other things.
Upstream looks abandoned, so we should probably investigate moving off of this package at some point (although this isn't a particularly complex, so I'm not that worried).
Either way, I'm just trying to get the package back into shape now (i.e. CI working, more modern package standard, etc). Pretty sure this should have no runtime impact on the package, which is good because the monolith pulls in
dev-master.I'll look to Rector this into PHP 8.3 shape in a new PR when the test harness is working.
Deployment Notes
I'm expecting that Dependabot will go on a mini rampage of PR bumps when this is merged, as I've moved us from PHP 7.3 to PHP 8.2.