All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ian Jackson <ian.jackson@eu.citrix.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps
Date: Mon, 17 Jul 2017 18:06:31 +0100	[thread overview]
Message-ID: <22892.61079.527978.196143@mariner.uk.xensource.com> (raw)
In-Reply-To: <20170713144355.GE1587@perard.uk.xensource.com>

Anthony PERARD writes ("Re: [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps"):
> On Thu, Jul 13, 2017 at 02:28:11PM +0100, Ian Jackson wrote:
> > I think this is not a critical problem, but fixing it would be nice at
> > some point.
> 
> The subunit stream contains the timestamps, so it just a matter of
> having substep_* taking it as an argument.

Ah.  Well, that can wait.

> > > +sub subunit_sanitize ($) {
> > > +    my ($testname) = @_;
> > > +    $testname =~ s/ /_/g;
> > > +    return $testname;
> > > +}
> > 
> > This function should have a more specific name.  Also it needs to be a
> > whitelist.
> 
> What about subunit_sanitize_testname?

SGTM.

> What kind of whitelist? What should it includes?

I mean: currently, you replace spaces with _s.  But you ought to have
a specific list of characters that you allow without replacing.

It doesn't help that osstest doesn't have an official character set
for testids.  Existing testids (other than various junk) contain
all of
    [^;_.()/~0-9a-zA-Z-]
(in Perl regexp charset syntax).

I suggest permitting all of those except ; which seems to have been a
mistake and has now been eliminated.  You may want to permit [ ] too.

> > Does the subunit protocol insist that
> > the spaces are single spaces ?  If not you need to use \s+.  You may
> > want to use the extended regexp syntax.
> 
> Looking at a description of the protocol and at the subunit code, does
> are single spaces.

Fair enough.

> What do you mean by "extended" ? Maybe operator like /.+?/, or maybe
> /(?<NAME>pattern)/ ?

I mean //x.  See "/x" in perlre(1).  This is a matter of taste.

> multipart just describes how the following lines are formated, it would
> have the 'content-type:' and the size of the output. non-multipart is
> just followed by text, and ends with '\n]\n' (both format do).
> 
> I don't think the code needs to care about it, just that it may or
> may not be there.

Hrm.

> > Does the subunit protocol not have any escaping ?  (Ie, what happens
> > if a thing run as part of a subunit test actually generates a line of
> > log output "]" ?)  If it does havve some escaping you need to
> > de-escape it.
> 
> Without "multipart", there does not seems to be any escaping. With
> multipart, the size of the output is in the protocol, I could extend the
> parser take it into account. It's just more work.

I think you must be able to consume the "multipart" and use the size,
then.  Otherwise if a test case printed output containing "\n]\n", in
multipart format, you would mishandle it and get desynchronised.

> FYI, part of the protocol about the output (with the beginning of
> DETAILS been "\[( multipart)?" in the regex):
> 
> DETAILS ::= BRACKETED | MULTIPART
> BRACKETED ::= '[' CR UTF8-lines ']' CR
> MULTIPART ::= '[ multipart' CR PART* ']' CR
> PART ::= PART_TYPE CR NAME CR PART_BYTES CR
> PART_TYPE ::= Content-Type: type/sub-type(;parameter=value,parameter=value)
> PART_BYTES ::= (DIGITS CR LF BYTE{DIGITS})* '0' CR LF

Sounds like you have to parse the multipart counted parts, I'm afraid.

> > What are subunit v1 consumers supposed to do with 1. unknown keywords
> > 2. syntax errors ?
> > I doubt that the answer to (2) is to ignore them as you do here...
> 
> "unexpected lines [...] should be forwarded unaltered". That's is in the
> readme of python-subunit project.

"forwarded" where I wonder ?  I guess printing them to logm is fine.
But you shouldn't ignore them, I think.

> As for keywords that can exist, there is "tags:", but in the case of
> tempest, it describe which worker did a test, when there is several
> concurrent worker. There is also "progress:" which is not very usefull
> for osstest. There is maybe more keywords which are test result which I
> should probably find out what there are, but I've got at least the one
> used by Tempest.

Fair enough.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

  parent reply	other threads:[~2017-07-17 17:06 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-12 15:04 [OSSTEST PATCH v12 00/21] Have OpenStack tested on top of xen's master and libvirt's master Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 01/21] ts-openstack-deploy: Deploy OpenStack on a host with devstack Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 02/21] ts-openstack-tempest: Run Tempest to check OpenStack Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 03/21] ts-openstack-deploy: Set http proxy Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 04/21] TestSupport: provide target_https_mitm_proxy_cert_path Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 05/21] ts-openstack-deploy: set CURL_CA_BUNDLE Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 06/21] ts-openstack-deploy: Keep CURL_CA_BUNDLE when sudo is called Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 07/21] ts-openstack-deploy: Try to disable use of SYSTEMD Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 08/21] ts-kernel-build: Enable network related modules for Neutron Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 09/21] ts-openstack-deploy: Switch to Neutron for network Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 10/21] ts-openstack-deploy: Increase open fd limit for RabbitMQ Anthony PERARD
2017-07-19 10:28   ` Ian Jackson
2017-07-19 11:08     ` Anthony PERARD
2017-07-19 13:05       ` Ian Jackson
2017-07-19 13:25         ` Anthony PERARD
2017-07-19 13:38           ` Ian Jackson
2017-07-12 15:04 ` [OSSTEST PATCH v12 11/21] ts-openstack-deploy: Apply a Tempest patch Anthony PERARD
2017-07-12 15:04 ` [OSSTEST PATCH v12 12/21] ts-openstack-deploy: Ignore libvirt-python version and use latest Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 13/21] ts-openstack-tempest: Fix tempest invocation Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 14/21] ts-openstack-tempest: Update list of skipped tests Anthony PERARD
2017-07-19 10:29   ` Ian Jackson
2017-07-12 15:05 ` [OSSTEST PATCH v12 15/21] ts-openstack-deploy: Move logs to /var/log/openstack Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 16/21] ts-logs-capture: Capture OpenStack logs Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 17/21] ts-openstack-deploy: Increase devstack timeout Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 18/21] TestSupport: Implement target_cmd_subunit a subunit stream parser into substeps Anthony PERARD
2017-07-13 13:28   ` Ian Jackson
2017-07-13 14:43     ` Anthony PERARD
2017-07-13 18:11       ` Anthony PERARD
2017-07-17 17:12         ` Ian Jackson
2017-07-17 17:06       ` Ian Jackson [this message]
2017-07-12 15:05 ` [OSSTEST PATCH v12 19/21] ts-openstack-tempest: Use target_cmd_subunit Anthony PERARD
2017-07-13 13:28   ` Ian Jackson
2017-07-12 15:05 ` [OSSTEST PATCH v12 20/21] Create a flight to test OpenStack with xen-unstable and libvirt Anthony PERARD
2017-07-19 12:56   ` Ian Jackson
2017-07-20 17:36     ` Anthony PERARD
2017-07-12 15:05 ` [OSSTEST PATCH v12 21/21] make-flight: Increase dom0_mem for openstack flight Anthony PERARD

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=22892.61079.527978.196143@mariner.uk.xensource.com \
    --to=ian.jackson@eu.citrix.com \
    --cc=anthony.perard@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.