How to deal with PRs which can't be tested ?

[Nils Rückmann]

See: https://github.com/joomla/joomla-cms/pull/5741

ls there any guideline how Joomla want to treat those PRs which can't be tested because of their simplicity ?

[Troy]

looks like it just needs detailed testing instructions showing what to expect post / pre to me.

Bear

On 5/25/2015 10:35, Nils Rückmann wrote:

See: https://github.com/joomla/joomla-cms/pull/5741

ls there any guideline how Joomla want to treat those PRs which can't be tested because of their simplicity ?

--
You received this message because you are subscribed to the Google Groups "Joomla! CMS Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to joomla-dev-cm...@googlegroups.com.
To post to this group, send email to joomla-...@googlegroups.com.
Visit this group at http://groups.google.com/group/joomla-dev-cms.
For more options, visit https://groups.google.com/d/optout.

No virus found in this message.
Checked by AVG - www.avg.com
Version: 2015.0.5941 / Virus Database: 4347/9864 - Release Date: 05/25/15

[Macduff Mathebula]

How do I unsubscribe to this group?

On May 25, 2015 5:35 PM, "Nils Rückmann" <ma...@nueckman.de> wrote:

See: https://github.com/joomla/joomla-cms/pull/5741

ls there any guideline how Joomla want to treat those PRs which can't be tested because of their simplicity ?

--

[Nils Rückmann]

Am Montag, 25. Mai 2015 17:45:37 UTC+2 schrieb Bear:

looks like it just needs detailed testing instructions showing what to expect post / pre to me.

Bear

Detailed testing instructions? Like "Nothing will change, but it should be a bit faster"? 

 

[Michael Babker]

If part of the testing procedure is reproducing the issue, and it can't be reproduced, can you safely say that the patch fixes the issue?  In a lot of cases, it may be trivial, but that is a mildly important step in resolving issues.

--

[Sergio Manzi]

"Nothing will change, but it should be a bit faster" seems to me to be good testing instructions (actually "Nothing should change, but it should be a bit faster" is a wee better...)

[Nils Rückmann]

Am Montag, 25. Mai 2015 20:46:29 UTC+2 schrieb Michael Babker:

If part of the testing procedure is reproducing the issue, and it can't be reproduced, can you safely say that the patch fixes the issue?  In a lot of cases, it may be trivial, but that is a mildly important step in resolving issues.

Of course, you're right. We have to be carefull about every single line. But let's take this PR (https://github.com/joomla/joomla-cms/pull/5741) as example. Of course the testing instruction is not ideal. But should this be a reason why a tiny little change, which indeed increases the perfomance, is still not RTC after 4 month ?

[Nils Rückmann]

Am Montag, 25. Mai 2015 21:08:30 UTC+2 schrieb Sergio Manzi:

"Nothing will change, but it should be a bit faster" seems to me to be good testing instructions (actually "Nothing should change, but it should be a bit faster" is a wee better...)

I admit .. point for you ;) 

[Michael Babker]

If it doesn't get tests with adequate results, sure.  I've had patches take a year to get merged simply because they never got tested, and I've had commit rights for a while now ;-)

(Not trying to shoot down the change, don't know its impacts (can't read regex to save my life), just sharing my perspective as someone who reviews the queue and makes merge decisions)

--

[Vic Drover]

in the specific example of speed testing, what exactly is the problem measuring speed?

Cheers,

Victor Drover
Founder and CEO, Anything Digital
Co-founder, Watchful.li & jInbound.com
262-923-8200 ext. 0
Facebook: AnythingDigital | watchfulli | JInbound
Twitter: @VicDrover | @AnythingDig | @watchfulli | @JoomlaInbound

[Nils Rückmann]

Am Montag, 25. Mai 2015 23:13:54 UTC+2 schrieb Vic Drover:

in the specific example of speed testing, what exactly is the problem measuring speed?

Thanks Vic. That's exactly my point. I don't know "hordijk", but it's very frustrating that someones efforts are not being accepted only because of stubbornness.

[Vic Drover]

I don't know to whom or what you are referring, but here's my point: If a PR improves performance, that is measurable to some % on localhost for example. If not, the test might be: function operates as expected with no measurable change in speed". In the case of the latter, the test is "operate as normal" which can be described for whatever function/feature is being tinkered with.

Cheers,

Victor Drover
Founder and CEO, Anything Digital
Co-founder, Watchful.li & jInbound.com
262-923-8200 ext. 0
Facebook: AnythingDigital | watchfulli | JInbound
Twitter: @VicDrover | @AnythingDig | @watchfulli | @JoomlaInbound

--

[Michael Babker]

Absolutely nothing.  However, that specific PR also comes with a caveat statement that the regex caused the system to hit a timeout with a large dataset.  So from my perspective, it'd be nice to see if someone could reproduce that same condition with a positive test result.  The performance difference may be immeasurable for smaller cases (which are probably what most test against).

[Nils Rückmann]

Am Montag, 25. Mai 2015 23:25:38 UTC+2 schrieb Vic Drover:

I don't know to whom or what you are referring, but here's my point: If a PR improves performance, that is measurable to some % on localhost for example. If not, the test might be: function operates as expected with no measurable change in speed". In the case of the latter, the test is "operate as normal" which can be described for whatever function/feature is being tinkered with.

I'm not referring to a certain person. Well, it seems strange that Brian wasn't able to see the purpose of this PR. Its more like a general statement. Joomla should just find way to not slowing itself down. And lost PRs like this are one factor. So i asked if there's something to prevent this kind of slowdowns in the future. How to write a test instruction for something that seems obvious ? Like performance tweaks, CS fixed or whatever.

[Sergio Manzi]

;-)

BTW, you have all my sympathy: I too had a PR that seemed to be forgotten (maybe it really had) for months (it is now merged).

I think there should be a process in the handling of issues and PRs that guarantee this cannot happen. There should be a reasonable time-frame (something like 3 months, but this is just to give an order of magnitude...) for testing/code review after which a PRs/issue should either be Merged, Rejected (closed) or Put on hold. In no cases a PR should be kept "on hold" for more than another reasonable time-frame (e.g. 1 year).

Yes, there, can be situations for which an otherwise valid PR cannot be merged: in those cases, IMHO, we should apply a special "On hold" label to it and periodically an ad-hoc committee (PLT or whatever) should assess the state of those PRs/Issues.

Different story for PRs that are already flagged as RTC and have a milestone assigned: that's perfectly fair.

Cheers!

Sergio

[Nils Rückmann]

Am Dienstag, 26. Mai 2015 00:58:55 UTC+2 schrieb Sergio Manzi:

BTW, you have all my sympathy: I too had a PR that seemed to be forgotten (maybe it really had) for months (it is now merged).

Thanks for your sympathy, but this isn't my PR ;) 

[Sergio Manzi]

No prob: sympathy anyway!!  :-)

[Bakual]

I don't know how to read regex expressions and I don't know from memory what exactly that regex is supposed to do. So I can't judge the impact by just reviewing it. I would have to put in considerable time to figure that out, which I have no. So I have no clue what you did in this PR and would need detailed description what it does and how it could be tested.

Means provide step-by-step instructions how to see the difference in code and explain exactly where we have to look for the changes in behavior. It mentions "additional spaces between attributes". In which cases and which attributes (I suppose in the URL, but it doesn't say that). Always explain the change in words so that a regular user could understand it, or at least test it. Many testers are not developers.

Look at PRs from Nicholas (nikosdion) for excellent examples :)

[Nils Rückmann]

Am Dienstag, 26. Mai 2015 08:25:27 UTC+2 schrieb Bakual:

I don't know how to read regex expressions and I don't know from memory what exactly that regex is supposed to do. So I can't judge the impact by just reviewing it. I would have to put in considerable time to figure that out, which I have no. So I have no clue what you did in this PR and would need detailed description what it does and how it could be tested.

Means provide step-by-step instructions how to see the difference in code and explain exactly where we have to look for the changes in behavior. It mentions "additional spaces between attributes". In which cases and which attributes (I suppose in the URL, but it doesn't say that). Always explain the change in words so that a regular user could understand it, or at least test it. Many testers are not developers.

Look at PRs from Nicholas (nikosdion) for excellent examples :)

Again: It's not my PR.  

[brian teeman]

FYI

It massively helps if when testing a PR you confirm that you observed the issue and that the PR resolves the issue AND you then mark it as a successful test on the Issue Tracker

Simply saying it "looks good" really isnt enough.

[Nils Rückmann]

You mean exactly what i did ? http://issues.joomla.org/tracker/joomla-cms/5741