code review 2123044: net: disable tests for functions not available on windows

net: disable tests for functions not available on windows

[brainman, 2010-09-10 06:35:39]

Hello golang-dev@googlegroups.com,

I'd like you to review this change.

[brainman, 2010-09-10 06:56:08]

I am surprised, but it seems to me that "unix" scokets are not available under
windows. I've never had a need for it, but. Perhaps, I'm wrong here. Please,
correct me, if I'm wrong.

Alex

[r, 2010-09-10 07:11:52]

LGTM

but could you please add a comment on the GOOS checks to explain why

[Joe Poirier, 2010-09-10 07:30:02]

On 2010/09/10 07:11:52, r wrote:
> LGTM
> 
> but could you please add a comment on the GOOS checks to explain why


Wouldn't it be easier just to add the following to the 
pkg/Makefile, these are the ones that fail (comments 
copied from nacl):

ifeq ($(GOOS),windows)
NOTEST+=exec         # no pipe
NOTEST+=log          # no runtime.Caller
NOTEST+=net          # no network
NOTEST+=os           # many things unimplemented
NOTEST+=os/signal    # no signals
NOTEST+=path         # tree walking does not work
NOTEST+=syslog       # no network
NOTEST+=time         # no syscall.Kill, syscall.SIGCHLD for sleep tests
NOTEST+=websocket    # no network
endif

-joe

[brainman, 2010-09-10 07:47:13]

> Wouldn't it be easier just to add the following to the 
> pkg/Makefile, ...

I do want to run most "net" test, just not some.

Alex

[brainman, 2010-09-10 07:52:22]

Hello golang-dev@googlegroups.com, r, Joe Poirier (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-09-10 07:54:11]

On 2010/09/10 07:11:52, r wrote:
> LGTM
> 
> but could you please add a comment on the GOOS checks to explain why

Done. But I'll wait for more comments, perhaps someone will prove me wrong about
"unix" sockets.

Alex

[Joe Poirier, 2010-09-10 08:05:44]

On Fri, Sep 10, 2010 at 2:47 AM,  <alex.brainman@gmail.com> wrote:
>> Wouldn't it be easier just to add the following to the
>> pkg/Makefile, ...
>
> I do want to run most "net" test, just not some.
>

Okay, understood.

On a side note.

At some point in the not too distant future there needs to be some
brainstorming on how to separate the windows and non-windows
test files for packages that will require it.

A quick hack I've used - create the windows test file but name it and
the non-windows version with a non-test name then add the following
to the package's make file:

ifeq ($(GOOS),windows)
x:=$(shell cp ./t_windows.go ./windows_test.go)
else
x:=$(shell cp ./t_unix.go ./unix_test.go)
endif

-joe

[Joe Poirier, 2010-09-10 08:31:54]

LGTM

[brainman, 2010-09-10 09:40:26]

On 2010/09/10 08:05:44, Joe Poirier wrote:
> ... there needs to be some
> brainstorming on how to separate the windows and non-windows
> test files for packages that will require it.

I hope we don't need many, and code like the one in this CL (suggested by Russ),
should do the trick.

Alex

[brainman, 2010-09-10 09:46:43]

On 2010/09/10 07:30:02, Joe Poirier wrote:

> Wouldn't it be easier just to add the following to the 
> pkg/Makefile, these are the ones that fail (comments 
> copied from nacl):
> 
> ifeq ($(GOOS),windows)
> NOTEST+=exec         # no pipe
> NOTEST+=log          # no runtime.Caller
> NOTEST+=net          # no network
> NOTEST+=os           # many things unimplemented
> NOTEST+=os/signal    # no signals
> NOTEST+=path         # tree walking does not work
> NOTEST+=syslog       # no network
> NOTEST+=time         # no syscall.Kill, syscall.SIGCHLD for sleep tests
> NOTEST+=websocket    # no network
> endif
> 

On unrelated note. Do you think it is possible to change pkg/Makefile in a
similar way to get $GOROOT/src/all.bash to run to completion on Windows? If yes,
perhaps we should do that, to have windows build on "dashboard" to pass. This
way we'll have all our existing code running in test. At the same time, as we
make remaining packages and tests work, we'll remove these lines one by one
until they are all gone.

Alex

[Joe Poirier, 2010-09-10 10:14:12]

On Fri, Sep 10, 2010 at 4:46 AM,  <alex.brainman@gmail.com> wrote:
> On 2010/09/10 07:30:02, Joe Poirier wrote:
>
>> Wouldn't it be easier just to add the following to the
>> pkg/Makefile, these are the ones that fail (comments
>> copied from nacl):
>
>> ifeq ($(GOOS),windows)
>> NOTEST+=exec         # no pipe
>> NOTEST+=log          # no runtime.Caller
>> NOTEST+=net          # no network
>> NOTEST+=os           # many things unimplemented
>> NOTEST+=os/signal    # no signals
>> NOTEST+=path         # tree walking does not work
>> NOTEST+=syslog       # no network
>> NOTEST+=time         # no syscall.Kill, syscall.SIGCHLD for sleep
>
> tests
>>
>> NOTEST+=websocket    # no network
>> endif
>
>
> On unrelated note. Do you think it is possible to change pkg/Makefile in
> a similar way to get $GOROOT/src/all.bash to run to completion on
> Windows?

Yes, the windows build (./all.bash) runs without test failures using the
NOTEST list above in pkg/Makefile.

> If yes, perhaps we should do that, to have windows build on
> "dashboard" to pass. This way we'll have all our existing code running
> in test. At the same time, as we make remaining packages and tests work,
> we'll remove these lines one by one until they are all gone.
>

Not so sure about letting the dashboard show that windows passes when
in fact it doesn't. People may think things are working when in fact they're
not.

-joe

[brainman, 2010-09-10 11:37:07]

On 2010/09/10 10:14:12, Joe Poirier wrote:
> Not so sure about letting the dashboard show that windows passes when
> in fact it doesn't. People may think things are working when in fact they're
> not.

I don't think it means much. It is "experimental".

On the other hand, we'll be able to see when build is broken.

Alex

[rsc1, 2010-09-10 17:11:48]

I'm in favor of adding necessary NOTEST lines
to make the windows build pass.  It's not for
advertising status to the world but so that we can
see when a regression happens and pinpoint the
CL that broke something that previously worked.

Russ

[Joe Poirier, 2010-09-10 17:25:11]

On Fri, Sep 10, 2010 at 12:11 PM, Russ Cox <rsc@google.com> wrote:
> I'm in favor of adding necessary NOTEST lines
> to make the windows build pass.  It's not for
> advertising status to the world but so that we can
> see when a regression happens and pinpoint the
> CL that broke something that previously worked.
>
> Russ
>

Okay, I'll create the cl.

-joe

[rsc1, 2010-09-10 17:32:15]

LGTM

[brainman, 2010-09-12 02:02:34]

*** Submitted as http://code.google.com/p/go/source/detail?r=7d47646ad41b ***

net: disable tests for functions not available on windows

R=r, Joe Poirier, rsc
CC=golang-dev
http://codereview.appspot.com/2123044

Committer: Alex Brainman <alex.brainman@gmail.com>

[brainman, 2010-09-12 02:05:42]

On 2010/09/10 17:25:11, Joe Poirier wrote:
> On Fri, Sep 10, 2010 at 12:11 PM, Russ Cox <mailto:rsc@google.com> wrote:
> > I'm in favor of adding necessary NOTEST lines
> > to make the windows build pass. &nbsp;It's not for
> > advertising status to the world but so that we can
> > see when a regression happens and pinpoint the
> > CL that broke something that previously worked.
> >
> > Russ
> >
> 
> Okay, I'll create the cl.
> 

I took liberty to remove net from NOTEST list as part of this CL without your
confirmation. I hope it is as expected.

Alex

[Joe Poirier, 2010-09-12 02:56:12]

> I took liberty to remove net from NOTEST list as part of this CL without
> your confirmation. I hope it is as expected.
>
> Alex
>

Oh yeah...no problem.

-joe