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.

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

Some improvements in tests #6: #13414

Merged
merged 2 commits into from
Jun 10, 2017
Merged

Some improvements in tests #6: #13414

merged 2 commits into from
Jun 10, 2017

Conversation

frankmayer
Copy link
Contributor

@frankmayer frankmayer commented Dec 29, 2016

Summary of Changes

  • merged unset() calls
  • declared a few properties

Testing Instructions

Code review only.
A lot of unset() merging. Except for 6-7 property declarations. Should be easy to review, though.
PRIORITY: LOWEST

Unified view for reviewing is recommended.

Documentation Changes Required

None

- declared some properties
- merged  unset() calls
@laoneo
Copy link
Member

laoneo commented Dec 30, 2016

@frankmayer I have another request. Can you also please exclude administrator/com_media from your cleanup please. We are redoing the new media manager and are touching almost any file there. So it will help us to stay up to date with staging.

@frankmayer
Copy link
Contributor Author

@laoneo (You're killing me... 😄). But of course, I'll exclude that, too, for future work and I will also check the current ones.
But it would also be nice if my existing PR's could finally be reviewed/merged. I have put a lot of work into all of this, and also did a lot of work splitting the really big ones up into more logical chunks to help you people review them.
There are a lot of smaller ones or ones with only one specific type of change, which should be relatively easy to review. When those are merged, I can also move on with conflict resolving of the initial big ones, which will result a lot less changes to finally review those ones, too.
Note, that I am aware that, with my PR's I have also put a lot of weight on the shoulders of the reviewers, but these are one time cleanups that would have to be done either way, to improve Joomla. So, please do help me, help you (Joomla Project ❤️).

@Quy
Copy link
Contributor

Quy commented May 29, 2017

It would be nice to limit this PR to unset only. I am unsure of the declaration of properties to give this a successful test.

@frankmayer
Copy link
Contributor Author

@Quy You cannot test this in the frontend. These are all changes to the testsuite.
I think, only code review is possible here, and since the tests do pass, it is a sign that those changes did not do any harm.

# Conflicts:
#	tests/unit/suites/libraries/joomla/filter/JFilterOutputTest.php
#	tests/unit/suites/libraries/joomla/uri/JURITest.php
@Quy
Copy link
Contributor

Quy commented May 29, 2017

I have tested this item ✅ successfully on d9b9ae5

Code review.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13414.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on d9b9ae5

on code review


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13414.

@ghost
Copy link

ghost commented May 31, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 31, 2017
@rdeutz rdeutz merged commit af3d2bf into joomla:staging Jun 10, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 10, 2017
@rdeutz rdeutz added this to the Joomla 3.7.3 milestone Jun 10, 2017
@frankmayer frankmayer deleted the improvements-in-tests-6 branch June 12, 2017 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants