Re: [LTP] [PATCH] Add goals of patch review and tips

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> >> +1. Prevent false positive test results
> >> +2. Prevent false negative test results
> >> +3. Make future changes as easy as possible
> >
> > I would say that number 3 maybe be a bit controversial, I've seen cases
> > where attempts to futureproof the code caused steep increase in the
> > test
> > complexity. So maybe:
> >
> > 3. Keep the code as simple as possible as well as futureproof
> 
> Perhaps just
> 
> 3. Keep the code as simple as possibe, but no simpler
> 
> This is possibly paraphrasing Einstein:
> https://quoteinvestigator.com/2011/05/13/einstein-simple/
> 
> 
> NOTE: I think future proofing is actually very dangerous. What I
>       probably meant was
> 
>       3. Keep the code as simple as possible, while maintaining optionality,
>          but if there appears to be a disproportionate increase in complexity
>          for an increase in optionality then simplicity takes priority because
>          identifying relevant optionality is hard.
> 
>       but "optionality" does not have a nice dictionary definition. I guess
>       you could substitute it with "freedom". In any case it's not something I
>       would want to write in documentation. There is no easy way to
>       express it.

That sounds way to complicated, unfortunately reality is often
complicated and cannot be overly simplified.

So I would go with the simple paraphrase to Einstein, that is short and
to the point.

> >> +## How to get patches merged
> >> +
> >> +Once you think a patch is good enough you should add your Reviewed-by
> >> +tags. This means you will get some credit for getting the patch
> >> +merged. Also some blame if there are problems.
> >
> > Maybe we should mention the Tested-by: tag explicitly here as well.
> 
> I'm not sure how we interpret Tested-by when deciding to merge; does it
> mean someone is happy for the test to be merged or not?
> 
> Should someone add both tags if they have reviewed and tested it?

Tested-by: means that someone actually tried the test and that it did
what it was supposed to do. This has obvious meaning for reproducers,
and yes for a reproducer you can add both tags and both are meaningful.

For regular tests Tested-by does not have that much value I guess.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp