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

Teamcity styling #350

Merged
merged 32 commits into from
Aug 2, 2021
Merged

Teamcity styling #350

merged 32 commits into from
Aug 2, 2021

Conversation

lukeraymonddowning
Copy link
Member

@lukeraymonddowning lukeraymonddowning commented Jul 16, 2021

Q A
Bug fix? no
New feature? yes

This PR aims to add Pest styling to the PhpStorm TeamCity Logger. Here are some output examples:

Screenshot 2021-07-16 at 17 53 09

Screenshot 2021-07-16 at 17 53 50

Screenshot 2021-07-16 at 17 54 09

Screenshot 2021-07-16 at 17 55 19

Screenshot 2021-07-16 at 17 55 57

@lukeraymonddowning lukeraymonddowning marked this pull request as ready for review July 16, 2021 16:59
@lukeraymonddowning lukeraymonddowning linked an issue Jul 16, 2021 that may be closed by this pull request
@olivernybroe
Copy link
Member

Looks really great!

It's not possible for us to share the output with the normal console so we don't have to maintain the output in two places?

src/Logging/TeamCity.php Outdated Show resolved Hide resolved
@lukeraymonddowning
Copy link
Member Author

Looks really great!

It's not possible for us to share the output with the normal console so we don't have to maintain the output in two places?

Hadn't really thought about that but will look at the possibility of unifying the two 👍

@lukeraymonddowning
Copy link
Member Author

@olivernybroe we now defer to Collision's adapter for output. However, there seems to be a little issue with test failures showing the error output:

Screenshot 2021-07-19 at 13 44 16

I imagine this is to do with test listeners and the output helpers. Any ideas?

Copy link
Member

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

Nice!

I think the code looks great and really nice cleanup. I haven't tested it out locally yet, hopefully I get time to do that soon or someone else does it 👍

{
parent::__construct(null, $verbose, $colors, false, 80, false);
$this->phpunitTeamCity = new \PHPUnit\Util\Log\TeamCity(
Copy link
Member

Choose a reason for hiding this comment

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

I might be a bit blind, but what replaced the part which did this? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically nothing. We're firing all of the events that team city were firing manually, and now that we don't share their output at all, it became redundant. Can you see an issue with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've gone through and manually implemented some of the events. We have to do it manually so as not to give PHPStorm too much information (because then it writes to the console) but just enough to get it to display the correct icon. If we decorate the base TeamCity logger it writes out too much to the console.

tests/Visual/JUnit.php Show resolved Hide resolved
@lukeraymonddowning
Copy link
Member Author

@nunomaduro @olivernybroe I'm pretty happy with this PR now. Could do with somebody who has PhpStorm seeing if I've missed anything. The only quirk is on skipped tests:

Screenshot 2021-07-23 at 00 10 57

In order to get the skip indicators in PhpStorm, it has to output the "Test ignored." text. I can get it to go to the bottom using ob_start and ob_flush around the event printer, but I can't get the text to disappear completely. Thoughts?

@lukeraymonddowning
Copy link
Member Author

Follow-up: We can pass a space for the message on skipped tests, which is cleaner, and combined with ob_start results in just a bunch of new lines at the bottom of the console output:

Screenshot 2021-07-23 at 00 18 49

@olivernybroe
Copy link
Member

olivernybroe commented Jul 23, 2021

Hey @lukeraymonddowning, there is unfortunately something wrong 😢

When I have a failing test, it does not report the error message of it as part of the specific test case.
image
image
I have to go to all output to be able to see why it failed. However before you could see this on each specific test, which is really helpful when having multiple failing tests.

It also looks like the print of the status from the file is getting outputted the wrong place.
For example, when looking at the result of the first test of the next test group, it contains the output of how what passed and failed in the previous group.
image

Another issue found. When using the goto feature (pressing the file) it does not work on phpunit files anymore. This is because it now uses the pest_qn for location hint instead of php_qn when using phpunit tests

So after looking further into what the results showing in the wrong grouping is about, it seems like all the output from the collision printer is not being wrapped in TeamCity format. This is needed so phpstorm knows how to categorise stuff.
For example, if we take a look at the following teamcity message when a test fails that was comparing data.

##teamcity[testFailed name='it can get null as gold if no spies' message='Failed asserting that two strings are identical.' details=' /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Expectation.php:183|n /Users/olivernybroe/Code/nightlands-api/tests/Feature/GraphQL/Queries/ProfileByIdTest.php:61|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Factories/TestCaseFactory.php:151|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Concerns/Testable.php:285|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Support/ExceptionTrace.php:28|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Concerns/Testable.php:286|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Concerns/Testable.php:274|n /Users/olivernybroe/Code/nightlands-api/vendor/pestphp/pest/src/Console/Command.php:130|n ' duration='483' type='comparisonFailure' actual='|'monkey|'' expected='|'cat|'']

This contains a type=comparisonFailure which lets phpstorm show the beloved diff between the two results. It also contains the details argument, which gives us all the extra information needed and then the name, so we know which test to connect this to and a message to show the user. However with the changes here, it is not wrapped in the teamcity event and the types and so on is also not here.

I hope this feedback helps, sorry for the wall of text

@lukeraymonddowning
Copy link
Member Author

Thanks @olivernybroe! That's super helpful. Do you know how we could support the full teamcity events without having it print to the console?

@olivernybroe
Copy link
Member

@lukeraymonddowning not completely no 🙁

We might be able to do a wrapper around the out object given to collision, so we can get the output from it and wrap it in a team city format? 🤔

@nunomaduro
Copy link
Member

@lukeraymonddowning @olivernybroe Once this is ready, fell free to merge and release.

@lukeraymonddowning
Copy link
Member Author

@olivernybroe I've just gone and refactored all of the work here to clean it up nicely. Ready for another review whenever you get chance.

Copy link
Member

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

I am done testing this out now and it works really well! Amazingly done ❤️

@lukeraymonddowning lukeraymonddowning marked this pull request as ready for review August 2, 2021 11:45
@lukeraymonddowning lukeraymonddowning merged commit fa8a57f into master Aug 2, 2021
@lukeraymonddowning lukeraymonddowning deleted the teamcity-styling branch August 2, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output when run via PHPStorm looks like phpunit, not pest
3 participants