www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - github: What to do when unittests fail?

reply Andrej Mitrovic <none none.none> writes:
I've cloned Phobos just a few minutes ago, and I've tried to build it with
unittests, I'm getting these:

Warning: AutoImplement!(C_6) ignored variadic arguments to the constructor
C_6(...)
 --- std.socket(316) broken test ---
 --- std.regex(3671) broken test ---

So what's the procedure now? Do I have to first revert to some earlier version
of Phobos that has unittests that pass, before doing any edits? I want to edit
an unrelated module and change some code (actually change a unittest), and make
a pull request. This is for an already reported bug in bugzilla.
May 23 2011
next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Also, David, your multithreaded unittest is gonna fry my CPU! It's
getting hot these days.. :)
May 23 2011
prev sibling next sibling parent reply Dmitry Olshansky <dmitry.olsh gmail.com> writes:
On 24.05.2011 1:33, Andrej Mitrovic wrote:
 I've cloned Phobos just a few minutes ago, and I've tried to build it with
unittests, I'm getting these:

 Warning: AutoImplement!(C_6) ignored variadic arguments to the constructor
C_6(...)
   --- std.socket(316) broken test ---
   --- std.regex(3671) broken test ---

Windows build is polluted by these messages that meant something sometime ago (usually there was a test that failed, and it was commented out for now). Still it builds quite easily, though I skipped a couple of recent commits.
 So what's the procedure now? Do I have to first revert to some earlier version
of Phobos that has unittests that pass, before doing any edits? I want to edit
an unrelated module and change some code (actually change a unittest), and make
a pull request. This is for an already reported bug in bugzilla.

http://d.puremagic.com/test-results/ -- Dmitry Olshansky
May 23 2011
next sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
I guess all I needed was git checkout <hash>, or something like that.
But I have a different question:

Which branch am I supposed to work on if I want to create a pull
request? master, devel or something else?
May 24 2011
parent reply David Nadlinger <see klickverbot.at> writes:
On 5/24/11 2:37 PM, Andrej Mitrovic wrote:
 Which branch am I supposed to work on if I want to create a pull
 request? master, devel or something else?

master, then create a feature branch for your pull request (»git checkout -b spiffy-new-feature«). David
May 24 2011
next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
 On 5/24/11, David Nadlinger <see klickverbot.at> wrote:
 master, then create a feature branch for your pull request (»git
 checkout -b spiffy-new-feature«).

Ok, but what about the errors in my original post? I should make sure all unittests pass when I make a pull request, right? If that's required I think I'd have to first checkout an older commit. But I'm not sure which one. There's a bunch of them for dates May 16th and older listed here: http://d.puremagic.com/test-results/platform-history.ghtml?os=Win_32 E.g. this one: http://d.puremagic.com/test-results/test_data.ghtml?dataid=62436 It says: From git://github.com/D-Programming-Language/phobos 3b628ae..d1d8124 master -> origin/master I'm not sure what 3b628ae..d1d8124 means, should I do "git checkout d1d8124" or something similar to get this commit?

Pull requests should generally be based on the latest code. Just because it passed when dealing with the older code doesn't mean that your changes will pass with the newer code. If the autotester is failing, I'd suggest simply waiting until it's fixed before doing a pull request unless you're certain that your fix is completely unrelated to its failure. If the current code in git is failing, it should generally be fixed fairly quickly. The tests are currently fully passing on Windows. They're failing on the other OSes because of a compiler change which affected std.datetime (due to a bug which causes dmd to run out of memory when compiling the full unit tests with std.datetime included, std.datetime's unit tests aren't currently run on Windows, which is why they're not failing). If you're using dmd 2.053, that shouldn't be a problem, and Don has a fix prepared for the bug, so the autotester should be passing again soon. Regardless, I wouldn't advise making changes based on old code. They're less likely to be correct and more likely to cause merge issues. - Jonathan M Davis
May 24 2011
prev sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 5/24/11, Jonathan M Davis <jmdavisProg gmx.com> wrote:
 The tests are
 currently fully passing on Windows.

But the thing is, the autotester uses a changeset dated May 22nd, but the latest changeset on github (https://github.com/D-Programming-Language/phobos) is May 16th, and that is the one I've cloned from and it has failing unittests. So I'm not sure where to go from there.
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 5/24/11, David Nadlinger <see klickverbot.at> wrote:
 master, then create a feature branch for your pull request (=BBgit
 checkout -b spiffy-new-feature=AB).

Ok, but what about the errors in my original post? I should make sure all unittests pass when I make a pull request, right? If that's required I think I'd have to first checkout an older commit. But I'm not sure which one. There's a bunch of them for dates May 16th and older listed here: http://d.puremagic.com/test-results/platform-history.ghtml?os=3DWin_32 E.g. this one: http://d.puremagic.com/test-results/test_data.ghtml?dataid= =3D62436 It says:
From git://github.com/D-Programming-Language/phobos

I'm not sure what 3b628ae..d1d8124 means, should I do "git checkout d1d8124" or something similar to get this commit?
May 24 2011
prev sibling parent Don <nospam nospam.com> writes:
Dmitry Olshansky wrote:
 On 24.05.2011 1:33, Andrej Mitrovic wrote:
 I've cloned Phobos just a few minutes ago, and I've tried to build it 
 with unittests, I'm getting these:

 Warning: AutoImplement!(C_6) ignored variadic arguments to the 
 constructor C_6(...)
   --- std.socket(316) broken test ---
   --- std.regex(3671) broken test ---

Windows build is polluted by these messages that meant something sometime ago (usually there was a test that failed, and it was commented out for now).

Those messages are still meaningful. They are to remind people that the bugby tests haven't been fixed.
 Still it builds quite easily, though I skipped a couple of recent commits.

 
 So what's the procedure now? Do I have to first revert to some earlier 
 version of Phobos that has unittests that pass, before doing any 
 edits? I want to edit an unrelated module and change some code 
 (actually change a unittest), and make a pull request. This is for an 
 already reported bug in bugzilla.

http://d.puremagic.com/test-results/

May 29 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Ok so this seems to be the last stable checkout for windows:
http://d.puremagic.com/test-results/test_data.ghtml?dataid=63757

For phobos it says:
Head commit:
commit 7ed4331f37edf6c3f433bd71eeaaefe917a6651a

So how do I jump to this commit in git? I can't figure it out from the
progit book..
May 24 2011
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
 I guess all I needed was git checkout <hash>, or something like that.
 But I have a different question:
 
 Which branch am I supposed to work on if I want to create a pull
 request? master, devel or something else?

Generally what I do is create a separate branch for whatever I'm working on. When I'm ready to do a pull request, I push it to a branch on github and do the pull request from there. I always leave master on my machine so that it matches the master in the main repository, so it's easy to build whatever the current official code is. - Jonathan M Davis
May 24 2011
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
 On 5/24/11, Jonathan M Davis <jmdavisProg gmx.com> wrote:
 The tests are
 currently fully passing on Windows.

But the thing is, the autotester uses a changeset dated May 22nd, but the latest changeset on github (https://github.com/D-Programming-Language/phobos) is May 16th, and that is the one I've cloned from and it has failing unittests. So I'm not sure where to go from there.

Just grab what's on github. It's the latest. The autotester grabbed something from May 22nd last, because dsimcha accidentally committed some code to the main repository instead of his own. He reverted it. No more checkins have been made since then, so the autotester hasn't grabbed anything new yet. github always has the latest code. Besides, if you look at the output on github, it's giving the same output as your original post. Note that it's a printout saying that a test is broken _not_ a test failure. It's a reminder that a test (and possibly related code) needs to be fixed. No unit tests are actually failing. An AssertError is thrown whenever that happens. What's on github is fine. - Jonathan M Davis
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Ah, I thought it was an actual test failure. Yes, I should have
checked the build results, silly me. :)

Thanks for all the help Jonathan & others.
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Hmm I've gotten into some trouble. I've made a pull request for my
first fix from a new branch:
https://github.com/D-Programming-Language/phobos/pull/56

Then I wanted to do another fix so I created a new branch, made the
fix, pushed to my phobos fork on github. Then I did a pull request
from this new branch, but in the new pull request it also showed two
commits, the new one and the older one from the previous pull request.

Should I have made multiple commits for each fix and do only a single
pull request?
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
I closed down the second pull request btw.
May 24 2011
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-05-24 11:45, Andrej Mitrovic wrote:
 Hmm I've gotten into some trouble. I've made a pull request for my
 first fix from a new branch:
 https://github.com/D-Programming-Language/phobos/pull/56
 
 Then I wanted to do another fix so I created a new branch, made the
 fix, pushed to my phobos fork on github. Then I did a pull request
 from this new branch, but in the new pull request it also showed two
 commits, the new one and the older one from the previous pull request.
 
 Should I have made multiple commits for each fix and do only a single
 pull request?

If you do each fix on completely separate branches, then you shouldn't have this problem. It sounds like you didn't start with a clean branch. Each branch needs to be made off a clean master (as in it has none of your changes in it) or it'll have all of the changes that differ from the main repository's master. That's one reason to always keep a clean master and make no changes on it directly. Any branches that you make from it are going to be clean. Now, combining related changes into a single pull request (especially if they're small) is fine. In fact, if the changes are small and at all related, it would probably be preferrable, since then there's less administration to deal with. However, remember that each pull request can be accepted or rejected individually, so if there's any real chance that one set of changes would be accepted and another rejected (particularly if they're not really all that related), then they should probably be separate pull requests. But remember that there's always the possibility that you'll be asked to make adjustments to your pull request, so it's still possible for parts of your initial request to be accepted and others rejected. Just use your best judgement on whether your changes are related enough to be done as a single pull request or whether they should be done separately. - Jonathan M Davis
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
I think I know what happened. After I've pushed my first branch, I
immediately created a second branch while I was still in the first
branch.

I think I should have switched to the main branch and then created the
second branch.

Otherwise, these two changes were just sample code fixes which get
displayed in the docs. I guess you could say they're related, and
they're really small changes anyway.
May 24 2011
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On 2011-05-24 12:34, Andrej Mitrovic wrote:
 I think I know what happened. After I've pushed my first branch, I
 immediately created a second branch while I was still in the first
 branch.
 
 I think I should have switched to the main branch and then created the
 second branch.

When you create a branch, it's a copy of whatever branch you were on when you created the copy. A branch doesn't have to come from master. So, if you branched from a branch, then all of your changes from the first branch would be in the second.
 Otherwise, these two changes were just sample code fixes which get
 displayed in the docs. I guess you could say they're related, and
 they're really small changes anyway.

Well, documentation fixes in general can probably be lumped together - especially website documentation as opposed to ddoc - at least as long as they're not major changes. If you're rewriting whole sections of documentation, then you might want to do separate pull requests. But small documentation fixes would probably be better put in a single change request to avoid the administrative overhead of multiple pull requests. Where you really get into issues with combining pull requests is when you combine completed unrelated bug fixes, and it doesn't sound like you're doing that. - Jonathan M Davis
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
These two were from ddoc. They're just typo's that were fixed, simple
to review and merge with master.

But I might have gone overkill with the DPL.org pull request:
https://github.com/D-Programming-Language/d-programming-language.org/pull/9

For that one most errors that were fixed were just code sample typo's,
but in other instances I've added some missing documentation (they're
rather small additions). I hope it's not too much for a pull request,
otherwise I'll have to figure out how to separate them into multiple
pull requests.
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Also for one commit almost the entire module is labeled as changed,
I've no idea how that happened (perhaps too many changes confuses
github?). The commit in question is this one:
https://github.com/AndrejMitrovic/d-programming-language.org/commit/70fb8902ebd7a63fa88e06700d776fb5dc876d99
May 24 2011
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
 Also for one commit almost the entire module is labeled as changed,
 I've no idea how that happened (perhaps too many changes confuses
 github?). The commit in question is this one:
 https://github.com/AndrejMitrovic/d-programming-language.org/commit/70fb890
 2ebd7a63fa88e06700d776fb5dc876d99

My guess would be that you ended up changing the line endings. - Jonathan M Davis
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
Well my editor automatically converts everything to unix LF when
saving a file. I think this is the standard for Phobos and dpl.org
libs, right? If not, I'll set the editor to just follow whatever line
endings a file has.

It also converts tabs to spaces as well. I think I did see some mixed
use of tabs and spaces in some modules.
May 24 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
*everything = newlines
May 24 2011
prev sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
On 2011-05-24 13:58, Andrej Mitrovic wrote:
 Well my editor automatically converts everything to unix LF when
 saving a file. I think this is the standard for Phobos and dpl.org
 libs, right? If not, I'll set the editor to just follow whatever line
 endings a file has.
 
 It also converts tabs to spaces as well. I think I did see some mixed
 use of tabs and spaces in some modules.

Everything is supposed to be using unix line endings, but that doesn't mean that they all do - particularly if it's an older file which isn't edited very often and the person who has generally edited uses Windows. So, it's possible that the file you edited had Windows line endings. Changing it so that it has unix line endings isn't a problem. The only possible issue is that that makes it so that it's not as obvious what the changes you made are. - Jonathan M Davis
May 24 2011