Storage\Maildir fixes + enable unit tests #82
Conversation
There was a problem hiding this comment.
@fredden Thanks for your PR.
Please see information about DCO and signoff:
https://github.com/laminas/.github/blob/master/CONTRIBUTING.md#about-the--s-flag
We need it in order to accept your patch. Sorry for inconvenience.
Here you will see instruction how you can change it in your PR:
https://github.com/laminas/laminas-mail/pull/82/checks?check_run_id=524109291
General note to your changes, without looking into details:
you are using arguments in assertions in wrong order - the first is the expected value, and the second actual, so we should have usually something like that:
$this->assertSame(12, $class->something());not:
$this->assertSame($class->someting(), 12);The result is of course the same, but error message in case of failure will be misleading.
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
There was a problem hiding this comment.
@fredden Thanks for you effort! 👍 In general it's great, but I do not understand some changes. I've added also couple suggestions, so please have a look when you have time and let me know what you think. Thanks :)
| @@ -133,7 +133,7 @@ public function getSize($id = null) | |||
| public function getMessage($id) | |||
| { | |||
| // TODO that's ugly, would be better to let the message class decide | |||
| if ($this->messageClass === Message\File::class | |||
| if (\trim($this->messageClass, '\\') === Message\File::class | |||
There was a problem hiding this comment.
Can you explain this change, please? Do we have any tests for that? It looks like a bugfix as well.
There was a problem hiding this comment.
Do we have tests: yes. They're disabled by default at present. I'm working on these separately. I'll move this bugfix to that branch.
There was a problem hiding this comment.
@fredden before I start reviewing it again just a quick question: you were saying about extracting bugfixes into separate PR - is it still a plan? I can still see all changes here. Thanks!
There was a problem hiding this comment.
I was going to move all the maildir test fixes to a separate pull request, but decided to leave here. If I move the bugfixes to a separate pull request then the testsuite will fail here until that lands. (The tests were being skipped by default, but run now.)
I'm happy to go with your recommendation. (Moving to a separate pull request isn't a lot of work.)
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
These tests do not work properly and will be fixed separately to this changeset. Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
|
Thanks for all the feedback. I've a few points to work through and will then request another review. |
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
Signed-off-by: Dan Wallis <mrdanwallis@gmail.com>
michalbundyra
changed the title
Signed-off-by: Michał Bundyra <contact@webimpress.com>
|
Thanks, @fredden! |
Storage\Maildir fixes + enable unit tests
Signed-off-by: Michał Bundyra <contact@webimpress.com>
Forward port laminas#82
Description
Increase test coverage. While test coverage is low, the GitHub status check from coverage is reporting failure on every pull request. While this change alone is unlikely to bring coverage above the threshold, it'll help.