-
Notifications
You must be signed in to change notification settings - Fork 737
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
Run unit tests on github action #1882
Conversation
run: 'composer update --prefer-dist --no-interaction --no-progress --ansi ${{ matrix.composer_flags }}' | ||
|
||
- name: 'Run tests' | ||
run: 'vendor/bin/phpunit --group unit' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just running unit test for the moment as booting a functional stack is more complicated. Will be done in a following PR if you don't mind.
@@ -9,10 +9,12 @@ | |||
|
|||
/** | |||
* @internal | |||
* @group unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this annotation at the class level, mark all methods as unit test.
4cca136
to
0d039e3
Compare
dfb1eab
to
cd07036
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Left one question but should not block merging this PR! Please get it in.
|
||
$opts = ['testing_invalid_option' => true]; | ||
$index->create([], $opts); | ||
} | ||
|
||
/** | ||
* @group unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes unit as there is no call done on the elasticsearch stack. Mocking a client is sufficient as it's not used for this test
@@ -24,7 +25,8 @@ public function testProcessor(): void | |||
$rename = new Rename('foo', 'target.field'); | |||
$set = new Set('field4', 324); | |||
|
|||
$processors = new Pipeline($this->_getClient()); | |||
$client = $this->createMock(Client::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share some background on this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes a client mock is sufficient here to satisfy the pipeline object constructor.
No call is made on the elasticsearch server, as only the toArray()
method is tested here.
@@ -124,7 +124,7 @@ public function testParentId(): void | |||
} | |||
|
|||
/** | |||
* @group unit | |||
* @group functional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 Nice catch.
I just added a changelog. |
See result here: https://github.com/deguif/Elastica/actions/runs/395847967 or at the end of the PR in the checks section.
I had to make several changes in tests to fix unit tests that were functional tests, here's the result without the changes on test classes: https://github.com/deguif/Elastica/runs/1480767481