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

Exception line/file of where thrown plus add shortcut to composer for running tests with `composer test #148

Merged
merged 7 commits into from
May 12, 2022

Conversation

treehousetim
Copy link
Contributor

@treehousetim treehousetim commented May 3, 2022

easier unit test running. instead of vendor/bin/phpunit
add more complete exception payload to message

Fixes #147

robbieaverill
robbieaverill previously approved these changes May 4, 2022
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I've requested a review from one of the Raygun team too.

@treehousetim treehousetim changed the title add shortcut to composer for running tests with `composer test Exception line/file of where thrown plus add shortcut to composer for running tests with `composer test May 4, 2022
@Deacon-McIntyre
Copy link
Contributor

Thanks for taking a look @robbieaverill , we'll get this released this week 👍

@Deacon-McIntyre
Copy link
Contributor

Hi @treehousetim, thanks for submitting this! During testing I noticed that RaygunExceptionMessage is also used by the SendError function of Raygun4PHP, so the changes from this PR cause this error when attempting to use SendError:

Message: Error: Call to a member function getFile() on null
#0 Raygun4php\RaygunExceptionMessage::BuildErrorTrace called at [/repos/raygun4php/src/Raygun4php/RaygunExceptionMessage.php:27]
#1 Raygun4php\RaygunExceptionMessage::__construct called at [/repos/raygun4php/src/Raygun4php/RaygunMessage.php:27]
#2 Raygun4php\RaygunMessage::build called at [/repos/raygun4php/src/Raygun4php/RaygunClient.php:287]
#3 Raygun4php\RaygunClient::BuildMessage called at [/repos/raygun4php/src/Raygun4php/RaygunClient.php:102]
#4 Raygun4php\RaygunClient::SendError called at [/var/www/html/app/raygunSetup.php:38]
#5 ::{closure} called at [/var/www/html/app/index.php:23]

Are you able to update this to work for both SendError and SendException ?

@treehousetim
Copy link
Contributor Author

Is there a test for this? I ran the test and none of them caught this.

@Deacon-McIntyre
Copy link
Contributor

I encountered it via manual testing, but upon running the tests on your branch I do see the testSendErrorInvokesTransportTransmit test fails in a similar way. Perhaps something is different in our environments?

1) Raygun4php\Tests\RaygunClientTest::testSendErrorInvokesTransportTransmit
Error: Call to a member function getFile() on null

/repos/raygun4php/src/Raygun4php/RaygunExceptionMessage.php:43
/repos/raygun4php/src/Raygun4php/RaygunExceptionMessage.php:27
/repos/raygun4php/src/Raygun4php/RaygunMessage.php:27
/repos/raygun4php/src/Raygun4php/RaygunClient.php:287
/repos/raygun4php/src/Raygun4php/RaygunClient.php:102
/repos/raygun4php/tests/RaygunClientTest.php:263
phpvfscomposer:///repos/raygun4php/vendor/phpunit/phpunit/phpunit:97

@treehousetim
Copy link
Contributor Author

I'll look at this and submit a fix. Thanks!

refactor to support both errorException and exception for reporting
running test automatically when a pull request is opened or code is pushed to master
@treehousetim
Copy link
Contributor Author

@Deacon-McIntyre I took another look at this. Running the tests revealed the defect you mentioned.
I have fixed the issue and expanded the feature to include this for both errorExceptions and exceptions.

I've also added a github action on this repo that will automatically install composer and run tests for code push and pull request opening. should add a nice safety net and a self-help workflow to PRs

thanks!

@Deacon-McIntyre
Copy link
Contributor

Hi @treehousetim , thanks for making those changes, and adding a GitHub action! I can confirm the tests all pass on my end.

However, when manually testing (plain PHP and HTML app, no frameworks), the errors I get coming through to Raygun have a null as the first stack trace item, like this:

"StackTrace": [
        null,
        {
            "LineNumber": 23,
            "ClassName": "Division by zero",
            "FileName": "/var/www/html/app/index.php",
            "MethodName": ""
        },
        {
            "LineNumber": 23,
            "ClassName": "",
            "FileName": "/var/www/html/app/index.php",
            "MethodName": "{closure}"
        }
    ],

Does this happen for you too? Are you able to have another look and see why it might be happening?

@treehousetim
Copy link
Contributor Author

@Deacon-McIntyre - fixed.

@treehousetim
Copy link
Contributor Author

It would be ideal to have a test to ensure there are no empty entries for the stack trace so I've added one.

Copy link
Contributor

@Deacon-McIntyre Deacon-McIntyre left a comment

Choose a reason for hiding this comment

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

Thanks for your efforts here @treehousetim!

@Deacon-McIntyre Deacon-McIntyre merged commit ab31a7c into MindscapeHQ:master May 12, 2022
@treehousetim
Copy link
Contributor Author

Hi @Deacon-McIntyre , what's the release cadence for a change like this?

@Deacon-McIntyre
Copy link
Contributor

@treehousetim If not today it will be out next week

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