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

Log error response message #221

Merged
merged 18 commits into from
Dec 19, 2022
Merged

Log error response message #221

merged 18 commits into from
Dec 19, 2022

Conversation

ghostwriter
Copy link
Contributor

  • If GitHub responds with a status code >= 200 && <= 299 the application will return the html_url for the release.
  • Otherwise, the application checks for message field describing the error

@Ocramius Ocramius changed the base branch from 1.20.x to 1.21.x December 16, 2022 13:18
@Ocramius Ocramius added this to the 1.21.0 milestone Dec 18, 2022
@Ocramius Ocramius self-assigned this Dec 18, 2022
@Ocramius
Copy link
Member

Infection CI failures unrelated: seems to be a regression from #220 which somehow didn't trip during PR phase 🤷

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢 here meanwhile

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Urgh, wait, no - I was bamboozled by CTRL+F not working in github logs 🤦


9) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] Concat

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation(PHP_EOL . 'Failed to create release through GitHub API;' . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
 }


10) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] ConcatOperandRemoval

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation(PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . PHP_EOL . 'Status code: %s' . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
 }


15) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] ConcatOperandRemoval

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
 }


16) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] Concat

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . 'Message: %s' . PHP_EOL . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
 }


17) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] ConcatOperandRemoval

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . PHP_EOL, $statusCode, $errorResponseData['message']);
     }
 }


18) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] Concat

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . PHP_EOL . 'Message: %s', $statusCode, $errorResponseData['message']);
     }
 }


19) /github/workspace/src/Github/Api/V3/CreateReleaseThroughApiCall.php:74    [M] ConcatOperandRemoval

--- Original
+++ New
@@ @@
             return new Uri($responseData['html_url']);
         }
         $errorResponseData = typed($responseBody, shape(['message' => non_empty_string()]));
-        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s' . PHP_EOL, $statusCode, $errorResponseData['message']);
+        invariant_violation('Failed to create release through GitHub API;' . PHP_EOL . 'Status code: %s' . PHP_EOL . 'Message: %s', $statusCode, $errorResponseData['message']);
     }
 }

In practice, my reccomendation:

  • remove most concatenations, especially no need for PHP_EOL
  • make sure that code path is covered - seems to really not be hit

@ghostwriter
Copy link
Contributor Author

The code path is covered, infection doesn’t recognize it was recently fixed sebastianbergmann/php-code-coverage#969 but not yet released.

Correct me if I'm wrong, If infection recognized Throw_ mutants correctly, it would recognize that all of the Concat* mutants would cause this test to fail because it assert that we got a specific exception message.

@Ocramius
Copy link
Member

I can also wait for sebastianbergmann/php-code-coverage#969

Doing the same for Roave/BetterReflection#1320

@ghostwriter
Copy link
Contributor Author

Sounds good to me.

Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
psalm-baseline.xml Outdated Show resolved Hide resolved
psalm-baseline.xml Outdated Show resolved Hide resolved
psalm.xml.dist Outdated Show resolved Hide resolved
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Comment on lines 51 to 52
/** @return iterable<int, array<array-key,array<string, mixed>>> */
public function provideInvalidPayload(): iterable
Copy link
Member

Choose a reason for hiding this comment

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

The reason why this was failing is that list<> contains an "empty" list type too.

As suggested, this can be iterable<int, array{array<string, mixed>}>`, which enforces the existence of the first array key inside each data provider entry 👍

Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Signed-off-by: Nathanael Esayeas <nathanael.esayeas@protonmail.com>
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Thanks @ghostwriter!

@Ocramius Ocramius merged commit b0db160 into laminas:1.21.x Dec 19, 2022
@ghostwriter ghostwriter deleted the feature/error-response branch December 19, 2022 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants