checkbox-dev team mailing list archive

[Zygmunt Krynicki <zygmunt.krynicki@xxxxxxxxxxxxx>]

NOTE: I've moved this to the public list, no need to talk in private about this.


On Tue, Feb 10, 2015 at 6:35 PM, Daniel Manrique
<daniel.manrique@xxxxxxxxxxxxx> wrote:
> On Tue, Feb 10, 2015 at 12:29 PM, Zygmunt Krynicki
> <zygmunt.krynicki@xxxxxxxxxxxxx> wrote:
>>
>>
>> On Tue, Feb 10, 2015 at 4:51 PM, Daniel Manrique
>> <daniel.manrique@xxxxxxxxxxxxx> wrote:
>>>
>>>
>>>
>>> On Tue, Feb 10, 2015 at 10:44 AM, Sylvain Pineau
>>> <sylvain.pineau@xxxxxxxxxxxxx> wrote:
>>>>
>>>> Hello
>>>>
>>>> To catch as early as possible job id modifications that could break test
>>>> plans depending on our providers,
>>>> I've creating a small bash script [1] that will run everyday at 00:30 on
>>>> lauma in the certification contab.
>>>>
>>>> It will only send mails to the mailing list if a job id has been
>>>> added/renamed/deleted the day before.
>>>> I checked that using an arbitrary date far in the past it triggers the
>>>> cron mail (see below) and that
>>>> nothing happens when ... there's nothing to report :)
>>>
>>>
>>>
>>> It's a great idea :)
>>>
>>> I remember checkbox legacy had some tests that would fail package building
>>> if certain checks in job files didn't pass, this gave a measure of
>>> protection against submitting bad job files. With the current structure this
>>> may be harder to do, but it may also be worth considering; if the tree
>>> contains weird jobs, packages will simply not build.
>>>
>>> Examples, we ensured that all job files were declared in setup.cfg, added
>>> to the potfiles catalog and added to the local.txt "master" local job file:
>>>
>>>     def test_job_files_in_setup_cfg(self):
>>
>>
>> What is this one doing?
>
> It checks that job files are declared in setup.cfg, this may not be
> needed now but it was back then, since the debian packaging stuff was
> responsible for collecting and installing job files. The consequence
> could be a missing job file that was referenced in a whitelist,
> causing problems.
>
>>
>>>
>>>     def test_job_files_in_potfiles(self):
>>
>>
>> We don't have a check like this. Now that we understand pot files and po
>> files in plainbox, we could do that
>>
>>>
>>>     def test_job_files_in_local_txt(self):
>>
>>
>> I suspect this one is totally obsolete now
>
> Yes, we don't use that anymore.
>
>>
>>>
>>>
>>> Also, we validated all jobs with a few basic checks. Some of these are
>>> done by plainbox (I recall because I worked a bit with Zygmunt on ensuring
>>> job validation considered these cases) but I don't remember if this is done
>>> at build time (plus as job definitions
>>
>>
>> Yes, we validate during build
>>
>>>
>>> have evolved, some of these checks are no longer even valid; in
>>> particular, many new jobs will break jobs_comply_with_schema and we no
>>> longer rely on parsable descriptions):
>>>
>>>     def test_job_files_valid(self):
>>
>>
>> I suspect this is something we already do
>
> This test is very simplistic, it just tries to load all the job files
> and if it comes up with 0 jobs, then something is horribly broken :)
> (but just 1 loaded job would make this test happy).
>
>>
>>>
>>>     def test_all_messages_have_name(self):
>>
>>
>> Messages?
>
> Remember in checkbox-legacy jobs were internally called "messages"?
> that's why this method has this name. It just ensures all jobs have a
> name: attribute. This was migrated to id and I'm pretty sure plainbox
> would complain if a job had no id :)
>
>>
>>>
>>>     def test_all_messages_have_command_or_description(self):
>>>     def test_shell_jobs_have_description(self):
>>>     def test_shell_jobs_with_root_have_needed_environ(self):
>>
>>
>> We do those
>>
>>>
>>>     def test_jobs_comply_with_schema(self):
>>
>>
>> Schema?
>
> Particularly when uploading to hexr, jobs that had custom fields would
> crash the parser. This is no longer the case with plainbox, but back
> then we needed to ensure jobs complied with the "schema", defined as
> the list of known fields and their value types. This is clearly not
> needed with plainbox since we use free-form, "extensible" jobs now.
>
>>
>>>
>>>     def test_verify_interact_jobs_have_command(self):
>>
>>
>> We do this
>>
>>>
>>>     def test_verify_manual_jobs_have_parsable_description(self):
>>
>>
>> We explicitly don't try to do this as there are no hidden types of syntax in
>> descriptions in general.
>
> +1 on that :)
>
>>
>>
>> If anyone wants to port any of those over to plainbox they belong in the
>> unit validation chain.
>
> Cool! Some of them may still be relevant. My comment was geared
> towards doing this at build (or test) time to protect against building
> packages with borked data. But I'm glad to see we already do some of
> this (see, I kinda remembered we did!).

Thanks, I think we have all of those EXCEPT for the one that is
related to i18n, which is pretty interesting.
We could now write a simple validation method for FileType with
unit.role == i18n and unit.path.endswith('POTFILES.in')

def files_mentioned_by_POTFILES_in(pathname):
    files_seen = []
    for line in open(pathname, 'rt', encoding='UTF-8'):
         if re.match("\[encoding: .+\]"): continue  # assume utf-8
         m = re.match("\[type: [^]]\s+(.+)")
         if not m:
             raise SyntaxError(...)
         files_seen.append(m.groups(1))
    return frozenset(files_seen)


def check_POTFILES_in(unit):
     assert unit.Meta.name == 'file'  and unit.path.endswith("POTFILES.in")
     assert unit.provider.base_dir is not None
     files_mentioned = files_mentined_bt_POTFILES_in(unit)
     for unit in [unit for unit in unit.provider.unit_list if
unit.Meta.name == 'file']:
          relative_path = os.path.relpath(unit.path, provider.base_dir)
          if relative_path not in files_mentioned:
               # self is the validator class
               self.warning("{} not mentioned by
{}".format(relative_path, unit.path)


The code above is hand-written and untested but the rough idea should
shine through, any takers?

Best regards
ZK