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

feat(teamcity): Add basic teamcity output format #141

Merged
merged 6 commits into from
Sep 11, 2020

Conversation

olivernybroe
Copy link
Member

@olivernybroe olivernybroe commented Jul 28, 2020

Q A
Bug fix? no
New feature? yes
Fixed tickets #140

This pull request adds team city output support to pest.

A future pr could be added, so the exception stack trace uses collision format.

@nunomaduro
Copy link
Member

Can you fix static analysis bugs buddy?

@olivernybroe
Copy link
Member Author

This should be ready now 👍

Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

LGTM! Do we need to (or can we) add some tests for this?

Just need to resolve the conflicts and this should be good to merge afaik.

@olivernybroe
Copy link
Member Author

We probably could add some tests to check the output. However the biggest problem with the test is that we have to mimic the fake test case class we create 🙂

Might be worth it thougg as we prob. could use it for other tests also

@olivernybroe
Copy link
Member Author

I have no idea how to fix this rector error tbh 👍

@owenvoke you know how?

@owenvoke
Copy link
Member

owenvoke commented Aug 17, 2020

Just investigating that issue, looks like it's due to the StaticCallOnNonStaticToInstanceCallRector rector.

It's caused by line 82 of the TeamCity class. Is there any reason for making that a static method? 🤔

Basically converting it to a non-static call fixes the issue, but I'm not sure if you made that static for a reason.

Oh, I see... it's static because it's a class name string. 😬 I'd change it to something like the following, but then PHPStan complains instead. 🙄

$fileName = forward_static_call([$suiteName, '__getFileName']);

@olivernybroe
Copy link
Member Author

@owenvoke Sounds like an issue with StaticCallOnNonStaticToInstanceCallRector maybe?

@TomasVotruba If you have a little time, could you help us on how to solve the rector error?

@TomasVotruba
Copy link

TomasVotruba commented Aug 18, 2020

Just flying by... Could you report and issue on Rector? With input/output/expected output.

If it's a blocker, check the @rectorphp README to exclude single rule (that's what I do to not burden the development process)

@olivernybroe
Copy link
Member Author

@TomasVotruba Cool, I have created an issue rectorphp/rector#3981 and will ignore the rule for now :)

@olivernybroe
Copy link
Member Author

I would really like to get this out as part of next version.

The phpstorm plugin has a bunch of issues which can't be resolved before this is added.

@owenvoke
Copy link
Member

I thought this was already merged, I think it's fine. 👍

@olivernybroe
Copy link
Member Author

@owenvoke I think it should have been merged, but I don't have permission 😅

@owenvoke owenvoke merged commit 1318bf9 into pestphp:master Sep 11, 2020
@owenvoke
Copy link
Member

Probably my bad for missing it. 😬 I've now merged it. 🎉

@olivernybroe
Copy link
Member Author

Thanks!

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.

4 participants