Issue 4621041: describing regression checking more explicitely (Closed)
|
Created:
12 years, 11 months ago by Janek Warchol Modified:
12 years, 11 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
Descriptiondescribing regression checking more explicitely
Patch Set 1 #
Total comments: 8
Patch Set 2 : fixup + making baseline #
Total comments: 9
Patch Set 3 : fix according to Graham #
Total comments: 1
Patch Set 4 : more style #MessagesTotal messages: 9
A small change to the CG to make things more obvious for the beginners. I hope that what i wrote is true ;) Rewording suggestions are most welcome. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:191: Ensure that currently built binary is the one without your changes. I think a beginner can overlook this step. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:201: in @file{lilypond-git/build/} directory. i remember that it wasn't obvious for me whether to run make test-baseline in lilypond-git/ or in lilypond-git/build/.
Sign in to reply to this message.
http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:191: Ensure that currently built binary is the one without your changes. On 2011/06/14 21:40:37, Janek Warchol wrote: > I think a beginner can overlook this step. Let's change that to: Run @code{make} with current git master without any of your changes. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:201: in @file{lilypond-git/build/} directory. On 2011/06/14 21:40:37, Janek Warchol wrote: > i remember that it wasn't obvious for me whether to run > make test-baseline > in lilypond-git/ or in lilypond-git/build/. ok. Could we make this: Before making changes to the code, establish a baseline for the comparison by going to the @file{lilypond-git/build/} directory and running: ? I prefer not to have text underneath an example. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:220: build/out/test-results/index.html hmm. Relative to the current directory, it's only out/test-results/index.html since the build directory itself has a build/ subdir (Colin calls this "elephants all the way down"), I think this could be confusing. What about specifying lilypond-git/build/... ?
Sign in to reply to this message.
@Graham: perfect! you caught exactly the subtleties which i was worrying that might be there! http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:191: Ensure that currently built binary is the one without your changes. On 2011/06/14 22:01:49, Graham Percival wrote: > On 2011/06/14 21:40:37, Janek Warchol wrote: > > I think a beginner can overlook this step. > > Let's change that to: > Run @code{make} with current git master without any of your changes. Done. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:201: in @file{lilypond-git/build/} directory. On 2011/06/14 22:01:49, Graham Percival wrote: > On 2011/06/14 21:40:37, Janek Warchol wrote: > > i remember that it wasn't obvious for me whether to run > > make test-baseline > > in lilypond-git/ or in lilypond-git/build/. > > ok. Could we make this: > > Before making changes to the code, establish a baseline for the comparison by > going to the @file{lilypond-git/build/} directory and running: > > ? I prefer not to have text underneath an example. Done. http://codereview.appspot.com/4621041/diff/1/Documentation/contributor/regres... Documentation/contributor/regressions.itexi:220: build/out/test-results/index.html On 2011/06/14 22:01:49, Graham Percival wrote: > hmm. Relative to the current directory, it's only > out/test-results/index.html > > since the build directory itself has a build/ subdir (Colin calls this > "elephants all the way down"), I think this could be confusing. > > What about specifying > lilypond-git/build/... > > ? Done. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:201: in @file{lilypond-git/build/} directory. Maybe it would be better placed at the bottom or over point 5? Should this be an Advanced note?
Sign in to reply to this message.
http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:201: @warning{ On 2011/06/15 08:14:54, Janek Warchol wrote: > Maybe it would be better placed at the bottom or over point 5? > Should this be an Advanced note? move it to an advanced note at the bottom of the page. It's certainly not important enough to be a warning; we want to "save" those for truly important stuff. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:207: @code{make} and @code{make check} it (without doing @code{make test-baseline} remove the parenthesis, just end with: @code{make} and @code{make check} it without doing @code{make test-baseline} again. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:224: available at: available (relative to the current build/ directory) at: http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:227: lilypond-git/build/out/test-results/index.html second thought: I think I prefer: out/test-results/index.html in order to not confuse people, see above comment.
Sign in to reply to this message.
As you wish :) http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:201: @warning{ On 2011/06/15 08:21:32, Graham Percival wrote: > On 2011/06/15 08:14:54, Janek Warchol wrote: > > Maybe it would be better placed at the bottom or over point 5? > > Should this be an Advanced note? > > move it to an advanced note at the bottom of the page. It's certainly not > important enough to be a warning; we want to "save" those for truly important > stuff. Done. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:207: @code{make} and @code{make check} it (without doing @code{make test-baseline} On 2011/06/15 08:21:32, Graham Percival wrote: > remove the parenthesis, just end with: > @code{make} and @code{make check} it without doing @code{make test-baseline} > again. Done. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:224: available at: On 2011/06/15 08:21:32, Graham Percival wrote: > available (relative to the current build/ directory) at: Done. http://codereview.appspot.com/4621041/diff/5001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:227: lilypond-git/build/out/test-results/index.html On 2011/06/15 08:21:32, Graham Percival wrote: > second thought: I think I prefer: > out/test-results/index.html > > in order to not confuse people, see above comment. Done.
Sign in to reply to this message.
LGTM, please make one change, then send me the final version for pushing. http://codereview.appspot.com/4621041/diff/9001/Documentation/contributor/reg... File Documentation/contributor/regressions.itexi (right): http://codereview.appspot.com/4621041/diff/9001/Documentation/contributor/reg... Documentation/contributor/regressions.itexi:215: available (relative to the current build/ directory) at: oops, that should be @file{build/} sorry about that, my fault.
Sign in to reply to this message.
2011/6/15 <percival.music.ca@gmail.com>: > LGTM, please make one change, then send me the final version for > pushing. > http://codereview.appspot.com/4621041/diff/9001/Documentation/contributor/reg... > Documentation/contributor/regressions.itexi:215: available (relative to > the current build/ directory) at: > oops, that should be > > @file{build/} > > sorry about that, my fault. I should've noticed that also. Patches attached. cheers, Janek
Sign in to reply to this message.
On Wed, Jun 15, 2011 at 12:33:54PM +0200, Janek Warchoł wrote: > I should've noticed that also. > > Patches attached. thanks, applied after squashing them into one patch. You might want to look into git rebase -i origin/master also, your git repo might be unhappy the next time you do git pull. Depends on whether it can auto-merge them or not. Cheers, - Graham
Sign in to reply to this message.
2011/6/15 Graham Percival <graham@percival-music.ca>: > thanks, applied after squashing them into one patch. issue closed. > You might want to look into > git rebase -i origin/master I know this. This change was originally 4 commits long, i reduced it to 2 :P Next time i'll reduce more. > also, your git repo might be unhappy the next time you do git > pull. Depends on whether it can auto-merge them or not. It is unhappy, but i deleted that branch and don't worry about it. thanks, Janek
Sign in to reply to this message.
|
