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

Refactor to allow unit testing and add first unit tests #231

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Refactor to allow unit testing and add first unit tests #231

merged 4 commits into from
Nov 30, 2018

Conversation

mrow4a
Copy link
Contributor

@mrow4a mrow4a commented Nov 17, 2018 •
edited
Loading

  • Refactor Application and add unit tests for it
  • Adjust to use drone and travis
  • Remove not needed sharing legacy code

@mrow4a mrow4a requested a review from DeepDiver1975 November 17, 2018 14:31
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch 6 times, most recently from f21ff13 to cd9acf1 Compare November 21, 2018 22:49
@mrow4a mrow4a changed the title [WIP] Refactor to allow unit testing and add unit tests Refactor to allow unit testing and add application unit tests Nov 21, 2018
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch 2 times, most recently from 58d92e1 to 5bb577c Compare November 21, 2018 23:10
@mrow4a mrow4a changed the title Refactor to allow unit testing and add application unit tests Refactor to allow unit testing and add first unit tests Nov 21, 2018
@codecov
Copy link

codecov bot commented Nov 21, 2018 •
edited
Loading

Codecov Report

Merging #231 into master will increase coverage by 5.23%.
The diff coverage is 62.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #231      +/-   ##
===========================================
+ Coverage         0%   5.23%   +5.23%     
- Complexity      286     294       +8     
===========================================
  Files            19      15       -4     
  Lines          1283    1146     -137     
===========================================
+ Hits              0      60      +60     
+ Misses         1283    1086     -197
Impacted Files Coverage Δ Complexity Δ
lib/Storage.php 0% <ø> (ø) 26 <0> (?)
lib/Helper.php 0% <ø> (ø) 33 <0> (?)
lib/Db.php 0% <ø> (ø) 33 <0> (?)
lib/Db/Storage.php 0% <ø> (ø) 3 <0> (?)
lib/Controller/SettingsController.php 0% <ø> (ø) 15 <0> (?)
lib/Filter.php 0% <ø> (ø) 7 <0> (?)
lib/Controller/DocumentController.php 1.84% <ø> (ø) 117 <0> (?)
lib/Db/Wopi.php 0% <ø> (ø) 9 <0> (?)
lib/AppConfig.php 0% <ø> (ø) 7 <0> (?)
lib/Http/DownloadResponse.php 0% <ø> (ø) 16 <0> (?)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e71069...468f910. Read the comment docs.

@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 21, 2018 •
edited
Loading

Can we have this merged as a start for the unit tests and code refactor with target on version 2.1.0? @phil-davis @individual-it @skshetry @DeepDiver1975

@mrow4a mrow4a requested a review from skshetry November 21, 2018 23:16
@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 21, 2018

Can we also after this PR disable travis?

@individual-it
Copy link
Member

Please rebase on the current master, they have been test related changes. E.g phpunit.xml was shifted to the root folder and the make file has more stuff in it

@skshetry
Copy link
Member

skshetry commented Nov 22, 2018 •
edited
Loading

I think @mrow4a rebased. Regarding the phpunit.xml file, we had decided to place it on the root of the repo. But, some apps(eg. password-policy) still have it in the tests, and I cannot point you to the specific discussion.

Maybe @individual-it @phil-davis can explain.

@phil-davis
Copy link
Contributor

I made PR owncloud/password_policy#166 to propose moving phpunit.xml in password_policy and to confirm that that is the current "standard" way that is should be.

@phil-davis
Copy link
Contributor

And I guess unit tests should be in a folder called tests/unit

@DeepDiver1975 @PVince81 please advise on "standard" unit test folder and XML file so we can do it "right" the first time, and avoid having apps with all different test folder structures.

DeepDiver1975
DeepDiver1975 requested changes Nov 22, 2018
View reviewed changes
Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Nice first approach - THX
Please adapt the comments
Keep it up!

.travis.yml Outdated Show resolved Hide resolved
js/share.js Show resolved Hide resolved
lib/appinfo/application.php Outdated Show resolved Hide resolved
tests/bootstrap.php Outdated Show resolved Hide resolved
tests/controller/documentcontrollertest.php Outdated Show resolved Hide resolved
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch 2 times, most recently from 78b71b7 to 6e4b78c Compare November 27, 2018 21:12
mrow4a
mrow4a commented Nov 27, 2018
View reviewed changes
templates/documents.php
@@ -10,9 +10,7 @@
</script>

<?php
style( 'richdocuments', 'share' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed share.js as not needed @DeepDiver1975

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meaning not used anywhere - was probably used by some legacy code back 2 years ago

@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch from 6e4b78c to 360fd7f Compare November 28, 2018 19:58
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch 3 times, most recently from 6503a1c to b78caa0 Compare November 28, 2018 20:55
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch from b78caa0 to 434ac79 Compare November 28, 2018 21:44
@mrow4a mrow4a force-pushed the refactor_for_unit_tests branch from 434ac79 to 468f910 Compare November 28, 2018 21:52
@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 28, 2018

@skshetry @DeepDiver1975 PR is ready for merging

@DeepDiver1975
Copy link
Member

I see travis still in here - migration to drone not yet finished?

@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 29, 2018

@DeepDiver1975 maybe we can make it scope of another PR?

@mrow4a
Copy link
Contributor Author

mrow4a commented Nov 29, 2018

@skshetry do you want to take care in follow-up PR?

@phil-davis
Copy link
Contributor

PR #230 was merged a week ago. That implemented drone.
We can remove .travis.yml in a separate PR, if you like.
That way this PR can be focused just on sorting out the actual unit tests, not on the CI system used to run them.

This is all part of issue #229

@skshetry skshetry closed this Nov 29, 2018
@skshetry skshetry reopened this Nov 29, 2018
@skshetry
Copy link
Member

Sorry, accidentally closed this. Looks good to me. Let @DeepDiver1975 approve this.

And, regarding removing the travis, I'll make a PR for that shortly.

@DeepDiver1975 DeepDiver1975 merged commit 7f5acad into master Nov 30, 2018
@delete-merged-branch delete-merged-branch bot deleted the refactor_for_unit_tests branch November 30, 2018 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skshetry skshetry skshetry approved these changes

@DeepDiver1975 DeepDiver1975 DeepDiver1975 approved these changes

Assignees

@mrow4a mrow4a

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mrow4a @individual-it @skshetry @phil-davis @DeepDiver1975