Skip to content

Add strict type on Notifications tests #15935

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
Merged
skjnldsv merged 1 commit into from
Jun 14, 2019
Merged

Add strict type on Notifications tests #15935

skjnldsv merged 1 commit into from
Jun 14, 2019

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Jun 12, 2019 •
edited
Loading

Followup #15912 (comment)

@skjnldsv skjnldsv added bug 3. to review Waiting for reviews labels Jun 12, 2019
@skjnldsv skjnldsv added this to the Nextcloud 17 milestone Jun 12, 2019
@skjnldsv skjnldsv self-assigned this Jun 12, 2019
@georgehrke
Copy link
Member

georgehrke commented Jun 12, 2019

I think we should add strict return types as well. :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 12, 2019

I think we should add strict return types as well. :)

Do_it_shia_laboeuf.gif

ChristophWurst
ChristophWurst reviewed Jun 12, 2019
View reviewed changes
@juliusknorr
Copy link
Member

juliusknorr commented Jun 13, 2019

Two remaining 😉

1) OCA\Comments\Tests\Unit\Controller\NotificationsTest::testViewSuccess
133 | TypeError: Argument 1 passed to Mock_INotification_fc0f9ea9::setUser() must be of the type string, null given, called in /drone/src/apps/comments/lib/Controller/Notifications.php on line 145
134 |  
135 | /drone/src/apps/comments/lib/Controller/Notifications.php:145
136 | /drone/src/apps/comments/lib/Controller/Notifications.php:118
137 | /drone/src/apps/comments/tests/Unit/Controller/NotificationsTest.php:153
138 |  
139 | 2) OCA\Comments\Tests\Unit\Controller\NotificationsTest::testViewNoFile
140 | TypeError: Argument 1 passed to Mock_INotification_fc0f9ea9::setUser() must be of the type string, null given, called in /drone/src/apps/comments/lib/Controller/Notifications.php on line 145
141 |  
142 | /drone/src/apps/comments/lib/Controller/Notifications.php:145
143 | /drone/src/apps/comments/lib/Controller/Notifications.php:118
144 | /drone/src/apps/comments/tests/Unit/Controller/NotificationsTest.php:216
145

@skjnldsv
Add strict type on Notifications tests
e9351ef 
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv skjnldsv force-pushed the fix/tests/notifications branch from 843f0d0 to e9351ef Compare June 13, 2019 12:58
@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 13, 2019

Done!

@skjnldsv skjnldsv requested a review from ChristophWurst June 13, 2019 17:14
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 14, 2019
@skjnldsv skjnldsv merged commit bf18f1e into master Jun 14, 2019
@skjnldsv skjnldsv deleted the fix/tests/notifications branch June 14, 2019 12:12
@blizzz
Copy link
Member

blizzz commented Jun 17, 2019

Also needs backports, doesn't it ?

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 19, 2019

Yes!

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 19, 2019

/backport to stable16

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 19, 2019
Closed
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jun 19, 2019

backport to stable16 in #16014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@georgehrke georgehrke Awaiting requested review from georgehrke

Assignees

@skjnldsv skjnldsv

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Milestone

Nextcloud 17

Development

Successfully merging this pull request may close these issues.

6 participants

@skjnldsv @georgehrke @juliusknorr @blizzz @ChristophWurst