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.


Check auto-tester first ;)
http://d.puremagic.com/test-results/

-- 
Dmitry Olshansky
May 23 2011
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 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 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


   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?
May 24 2011
prev sibling 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 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.


Yes, that's the idea.

 

 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.


 Check auto-tester first ;)
 http://d.puremagic.com/test-results/
May 29 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