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

Refactored integration test for ContentService #2757

Merged
merged 2 commits into from
Sep 5, 2019

Conversation

mikadamczyk
Copy link
Contributor

@mikadamczyk mikadamczyk commented Sep 4, 2019

Question Answer
JIRA issue N/A
Required by #2753
Improvement yes
New feature no
Target version 7.5
BC breaks no
Tests pass yes
Doc needed no

This refactor is a result of working on ContentService and a need to add some new tests and change the old ones in https://jira.ez.no/browse/EZP-30427

TODO:

  • Refactor tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@mikadamczyk mikadamczyk force-pushed the refactor-content-service-integration-test branch from 9a41f26 to 37f3306 Compare September 4, 2019 09:02
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Thanks @mikadamczyk 🎉 This is very much needed refactoring.

Overall please see SonarLint analysis and address issues related to your changes, according to our internal sync. Also please restore dropped FQCN in PhpDocs.

Some specifics:

eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
eZ/Publish/API/Repository/Tests/ContentServiceTest.php Outdated Show resolved Hide resolved
@mikadamczyk mikadamczyk force-pushed the refactor-content-service-integration-test branch from 6e95914 to b1107e2 Compare September 4, 2019 12:27
@pspanja
Copy link
Contributor

pspanja commented Sep 4, 2019

Just for info - tests were intentionally written in that style, because the idea was to extract the parts between /* BEGIN: Use Case */ and /* END: Use Case */ with a script and use it to automatically generate API examples for documentation. With this change the tests are not usable for that any longer, as the code does not read easily if embedded without context. So you could remove the markers as well.

BTW, the extraction script should still be somewhere in the repository.

@alongosz
Copy link
Member

alongosz commented Sep 4, 2019

Thanks @pspanja for your input! I didn't know that :)

@mikadamczyk
Copy link
Contributor Author

Thank you for your explanation @pspanja :)

@mikadamczyk mikadamczyk force-pushed the refactor-content-service-integration-test branch from b1107e2 to d34fb71 Compare September 4, 2019 14:42
@alongosz
Copy link
Member

alongosz commented Sep 5, 2019

No QA required - only integration test changes. Merging, thank you @mikadamczyk!

@alongosz alongosz merged commit 5d82e1a into 7.5 Sep 5, 2019
@alongosz alongosz deleted the refactor-content-service-integration-test branch September 5, 2019 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants