make check still brittle with doctests

Bug #661205 reported by Vincent Ladeuil
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Confirmed
High
Unassigned
Breezy
Fix Released
High
Jelmer Vernooij

Bug Description

We broke pqm again in a way that let it accept any submission.

The fix for https://bugs.edge.launchpad.net/bzr/+bug/626667 captures errors that produce *no* ouput for:

  $(PYTHON) -Werror -O ./bzr selftest --subunit $(tests)

Which works for all tests except for... doctests.

The case at hand led to selftest.log containing only:

**failed to get doctest for: bzrlib.pyutils
line 6 of the doctest for bzrlib.pyutils.calc_parent_name has an invalid option: '+SKIP'

which came from:

    """Determine the 'parent' of a given dotted module name and (optional)
    member name.

    Typical use is::

        >>> parent_mod, parent_member, final_attr = calc_parent_name(
        ... module_name, member_name) # doctest: +SKIP
        >>> parent_obj = get_named_object(parent_mod, parent_member)
        ... # doctest: +SKIP

    The idea is that ``getattr(parent_obj, final_attr)`` will equal
    get_named_object(module_name, member_name).

    :return: (module_name, member_name, final_attr) tuple.
    """

I'm landing a fix to comment this out to fix pqm willingness to
merge anything :)

Related branches

Vincent Ladeuil (vila)
Changed in bzr:
status: New → Confirmed
importance: Undecided → High
summary: - make check still brittle
+ make check still brittle with doctests
Jelmer Vernooij (jelmer)
tags: added: selftest
Revision history for this message
Martin Pool (mbp) wrote :

I was wondering if this is still active, but I think it is. The makefile has

check-nodocs: extensions
 set -e
 # Generate a stream for PQM to watch.
 -$(RM) -f selftest.log
 $(PYTHON) -Werror -O ./bzr selftest --subunit $(tests) | tee selftest.log
 # An empty log file should catch errors in the $(PYTHON)
 # command above (the '|' swallow any errors since 'make'
 # sees the 'tee' exit code for the whole line
 if [ ! -s selftest.log ] ; then exit 1 ; fi
 # Check that there were no errors reported.
 subunit-stats < selftest.log

There are a couple of problems with this:

* The shell will ignore non-zero return codes from everything but the last command in a pipeline. (You can tell zsh to do otherwise, but I don't think there's any portable way.) So anything that causes bzr to abort won't be seen.
* The subunit file having something in it is no guarantee that all the tests ran to completion.

If this bites us again I would be strongly inclined to just take out subunit until we have a way to run it that's inherently safe.

Revision history for this message
Vincent Ladeuil (vila) wrote :

This can still bite us but switching to tarmac should avoid the problem (by getting away from sh and avoiding the 'return code lost in pipe' issue).

Revision history for this message
John A Meinel (jameinel) wrote : Re: [Bug 661205] Re: make check still brittle with doctests

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/2/2011 1:35 AM, Vincent Ladeuil wrote:
> This can still bite us but switching to tarmac should avoid the problem
> (by getting away from sh and avoiding the 'return code lost in pipe'
> issue).
>

I don't think tarmac gets us away from "running sh", since it just runs
a command to run tests as well.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1J9FQACgkQJdeBCYSNAANiswCfXQu3XPDJPIVvFG1uHcJm+0bS
zY4An0A+5fi0LKyOUlkUlb7KWYft7MLw
=7mof
-----END PGP SIGNATURE-----

Revision history for this message
Vincent Ladeuil (vila) wrote :

> I don't think tarmac gets us away from "running sh", since it just runs a command to run tests as well.

I was considering that tarmac may allow us to run python code instead of sh and better control the intermediate steps, sorry for the confusion

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/3/2011 1:26 AM, Vincent Ladeuil wrote:
>> I don't think tarmac gets us away from "running sh", since it just
> runs a command to run tests as well.
>
> I was considering that tarmac may allow us to run python code instead of
> sh and better control the intermediate steps, sorry for the confusion
>

We could do that today with:

build:
  $(PYTHON) script.py

If you think things will actually be better that way, we can look into it.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1LHVgACgkQJdeBCYSNAAMhGQCgt157lMriHX8GGpst9il8bJGK
m54AoIvho6r4OA0hcWZzT1K6PhyJsayL
=SqU7
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

On 4 February 2011 08:25, John A Meinel <email address hidden> wrote:

> We could do that today with:
>
> build:
>  $(PYTHON) script.py
>
> If you think things will actually be better that way, we can look into
> it.

It seems a bit silly to write a Python script to wrap a script we
ourselves control.

What it needs is, I think:
 * selftest itself should fail if any tests fail, even with --subunit

and actually that's it, isn't it? The only reason we need to write to
a file as well as to stdout is so we can check if there are any
errors, but this is in fact a bit inherently dangerous.

Revision history for this message
Robert Collins (lifeless) wrote :

Perhaps you could just set pipefile in Makefile ?

Revision history for this message
Vincent Ladeuil (vila) wrote :

'pipefile' (or was it pipefail ?) was mentioned in the past but didn't make it, I can't remember precisely why...

Portability issues shouldn't really matter since the target is the pqm instance only...

Revision history for this message
Martin Pool (mbp) wrote :

On 4 February 2011 10:34, Robert Collins <email address hidden> wrote:
> Perhaps you could just set pipefile in Makefile ?

s//pipefail

This would make it bash-specific, and we're probably generally not
running bash, and it would need to be done on the same line, since
Make runs each line in a separate process. But otherwise yes.

The thing is if we make it exit nonzero on failure, we should not need
to have a pipeline at all, as far as I can see. Just emit subunit to
stdout and pqm should be able to use that both for the progress
display and to send to the user if it fails.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2/4/2011 1:27 AM, Martin Pool wrote:
> On 4 February 2011 10:34, Robert Collins <email address hidden> wrote:
>> Perhaps you could just set pipefile in Makefile ?
>
> s//pipefail
>
> This would make it bash-specific, and we're probably generally not
> running bash, and it would need to be done on the same line, since
> Make runs each line in a separate process. But otherwise yes.
>
> The thing is if we make it exit nonzero on failure, we should not need
> to have a pipeline at all, as far as I can see. Just emit subunit to
> stdout and pqm should be able to use that both for the progress
> display and to send to the user if it fails.
>

As I recall, wasn't the original issue that the subunit-foo that we were
running at the end of the pipe didn't exit non-zero for failing tests vs
corrupted stream?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1MFxsACgkQJdeBCYSNAANr+gCbBDDhwaw4Ea3XRm9iZ03abzfm
XOAAnRRe/XNvB+OKznws/uxw9MI5jqQi
=uF45
-----END PGP SIGNATURE-----

Revision history for this message
Martin Pool (mbp) wrote :

On 5 February 2011 02:11, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 2/4/2011 1:27 AM, Martin Pool wrote:
>> On 4 February 2011 10:34, Robert Collins <email address hidden> wrote:
>>> Perhaps you could just set pipefile in Makefile ?
>>
>> s//pipefail
>>
>> This would make it bash-specific, and we're probably generally not
>> running bash, and it would need to be done on the same line, since
>> Make runs each line in a separate process.  But otherwise yes.
>>
>> The thing is if we make it exit nonzero on failure, we should not need
>> to have a pipeline at all, as far as I can see.  Just emit subunit to
>> stdout and pqm should be able to use that both for the progress
>> display and to send to the user if it fails.
>>
>
> As I recall, wasn't the original issue that the subunit-foo that we were
> running at the end of the pipe didn't exit non-zero for failing tests vs
> corrupted stream?

That's right: a file with no errors in it isn't detected as being a
test failure. But it might have no errors because for whatever reason
we didn't end up running the whole test suite. That's why I think
it's inherently more risky to ignore the error code from bzr, and for
bzr to exit 0 even though it knows some tests failed.

Jelmer Vernooij (jelmer)
Changed in brz:
status: New → In Progress
importance: Undecided → High
assignee: nobody → Jelmer Vernooij (jelmer)
milestone: none → 3.0.0
tags: added: su subunit
Changed in brz:
status: In Progress → Fix Committed
Jelmer Vernooij (jelmer)
Changed in brz:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.