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

[TEST]: ImagineController functional test fixes #1087

Closed
wants to merge 1 commit into from
Closed

[TEST]: ImagineController functional test fixes #1087

wants to merge 1 commit into from

Conversation

nmuntyanov
Copy link

Q A
Branch? 2.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

It seems that Symfony\Component\BrowserKit\Client library was updated and since that
Symfony\Component\BrowserKit\Client::request() doesn't throws server's exceptions.
So, my fix is to check responses status codes instead checking for exceptions

@maximgubar
Copy link
Contributor

@nmuntyanov, there are failed builds on CI for your PR. could you please fix it ?

@maximgubar
Copy link
Contributor

and could you please provide a test-case on how to reproduce this issue ?

@nmuntyanov
Copy link
Author

nmuntyanov commented May 8, 2018

@maximgubar All that I was able to reproduce on my local environment was fixed by me in this PR.
The issue was inside the tests: because of that there is no composer.lock file - new Symfony component was installed (symfony/browser-kit I guess) and it has no backward compatibility.

@robfrawley
Copy link
Collaborator

robfrawley commented May 9, 2018

it has no backward compatibility

@nmuntyanov That isn't true, or if it is then Symfony has a bug that needs to be corrected (as there should be no breaking changes between the 4.0.0 release and the following 4.x.x releases. You need to test locally using multiple versions of Symfony, including dev-master, as our CI does.

That said, I suggest you rebase on top of the latest 2.0 branch (bringing in the changes merged in #1090) which should fix at least one of the failures I saw for your PR (I'm not sure about the other one though).

Note: please be sure to rebase and not merge when applying your changes on top of the current repo upstream.

@robfrawley
Copy link
Collaborator

robfrawley commented May 9, 2018

Oops, nevermind my prior comment. #1090 was the same fix you are attempting to fix here, but there was a way to accomplish it without removing our expected exceptions. Thanks though for you involvment in this project!

(Specifically, https://github.com/liip/LiipImagineBundle/pull/1090/files#diff-8ddc477a77a8b3fdbab01fd8e6231b95R47 fixed the issue you have here without editing the tests; see symfony/symfony#22890)

@robfrawley robfrawley closed this May 9, 2018
@nmuntyanov nmuntyanov deleted the liip-2.0 branch May 9, 2018 06:00
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.

None yet

3 participants