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.

Sign up for GitHub

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

Jump to bottom

LCH-3641: Add unit test to the Library #97

Merged
merged 3 commits into from
Feb 12, 2020
Merged

LCH-3641: Add unit test to the Library #97

merged 3 commits into from
Feb 12, 2020

Conversation

c0d39uru
Copy link
Contributor

@c0d39uru c0d39uru commented Dec 18, 2019 •
edited
Loading

  • Reviewing existing tests
    • Correcting those that are qualified to keep
    • Otherwise, rewriting them from scratch
  • Adding new tests to cover the remaining parts
  • Adding infection to gage the quality of the tests

If you want to run infection, you should install it first through make install command. For more information, please view the README.md file

Original ticket: https://backlog.acquia.com/browse/LCH-3641

@c0d39uru c0d39uru force-pushed the LCH-3641 branch 3 times, most recently from 5683cd5 to a7e4cb9 Compare December 31, 2019 02:34
@c0d39uru c0d39uru force-pushed the LCH-3641 branch 3 times, most recently from 93a0bef to 859ca64 Compare January 7, 2020 20:07
@c0d39uru c0d39uru changed the title WIP: LCH-3641 LCH-3641 Jan 7, 2020
@c0d39uru c0d39uru changed the title LCH-3641 LCH-3641: Add unit test to the Library Jan 7, 2020
Add Unit Tests to the library; add Makefile to facilitate different a…
714bbdb 
…ctions on it (dependency installation, run tests, etc.); add README.md to let the end-user know how to use it; add infection to evaluate tests quality
@kaynenh
Copy link
Contributor

kaynenh commented Jan 8, 2020

Initial overview of this looks good. Are we okay with the infection library not working on php 7.1 (failing tests error)? I believe @EclipseGc and @abarriosr may have additional thoughts around the code done here.

@c0d39uru
Copy link
Contributor Author

c0d39uru commented Jan 8, 2020 •
edited
Loading

Initial overview of this looks good. Are we okay with the infection library not working on php 7.1 (failing tests error)? I believe @EclipseGc and @abarriosr may have additional thoughts around the code done here.

You're right. As per Infection's composer.json, it needs "php": "^7.2.9", to run, but this is a dev requirement, not an actual one. So, there's no harm in letting it sit there.

jose-oliveira
jose-oliveira reviewed Jan 16, 2020
View reviewed changes
composer.json Outdated
"mockery/mockery": "^1.2"
"mockery/mockery": "^1.2",
"infection/infection": "^0.15.0",
"ext-json": "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency is added both as prod and dev.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jose-oliveira Mockery has been there for a while, I didn't add it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the ext-json one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jose-oliveira
Copy link
Contributor

Initial overview of this looks good. Are we okay with the infection library not working on php 7.1 (failing tests error)? I believe @EclipseGc and @abarriosr may have additional thoughts around the code done here.

You're right. As per Infection's composer.json, it needs "php": "^7.2.9", to run, but this is a dev requirement, not an actual one. So, there's no harm in letting it sit there.

Well, the harm is that we can't run tests on 7.1 with this dependency. If we aren't running Infection as part of our CI process, maybe we should require it in the make file?

@jose-oliveira jose-oliveira self-requested a review January 16, 2020 15:37
jose-oliveira
jose-oliveira suggested changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@jose-oliveira jose-oliveira left a comment

Choose a reason for hiding this comment

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

Code looks ok, but we need to figure out what to do with PHP 7.1 tests failing due to incompatibility with Infection version.

@c0d39uru c0d39uru closed this Jan 16, 2020
@c0d39uru c0d39uru reopened this Jan 16, 2020
@c0d39uru
Copy link
Contributor Author

That's right. I think we can safely remove infection for a while.

Remove ext-json from dev since it's already a dependency; move infect…
205d292 
…ion to makefile due to incampatibility with PHP version less than 7.3
@jose-oliveira jose-oliveira self-requested a review January 20, 2020 15:58
EclipseGc
EclipseGc reviewed Feb 11, 2020
View reviewed changes
src/SearchCriteria/SearchCriteriaBuilder.php
@@ -80,11 +72,8 @@ protected static function propertiesMap()
protected static function extractPropertyFromArray(string $propertyName, array $data, $default = '')
{
$map = self::propertiesMap();
if (!isset($map[$propertyName])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed the removal of this bit. @cleancode4ever pointed out that it was overly paranoid, and that with the way the system is formulated he couldn't actually test it meaning there wasn't effectively a way to ever invoke it.

EclipseGc
EclipseGc suggested changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@EclipseGc EclipseGc left a comment

Choose a reason for hiding this comment

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

Removing putEntities isn't going to work for us. It's postEntities() we never use. (also we don't use putEntity)

@c0d39uru
Copy link
Contributor Author

Removing putEntities isn't going to work for us. It's postEntities() we never use. (also we don't use putEntity)

Fixed!

@EclipseGc EclipseGc merged commit c77f4f2 into 2.x Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EclipseGc EclipseGc EclipseGc requested changes

@jose-oliveira jose-oliveira jose-oliveira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c0d39uru @kaynenh @jose-oliveira @EclipseGc