You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ponymail.apache.org by Daniel Gruno <hu...@apache.org> on 2021/06/02 17:32:00 UTC
Re: [incubator-ponymail-unit-tests] branch master updated: Allow
other versions to specifically make use of 'alternate'
I've made it so that the tests will pass with 56 specific format=flowed
issues in mind. We have the option of:
- Be strict and let foal fail forever
- Be flexible and have foal and pony differ in these specific cases
- Create a separate set of tests for both
I am leaning towards option 2, which is how it is right now. Option 1
means we can't use the tests for foal, and option 3 would mean we risk
not noticing when the two diverge.
For the places where the tests diverge (and need alternates), I've put a
top comment in each YAML specifying who/why. I think this is currently
the best compromise.
On 02/06/2021 19.27, humbedooh@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> humbedooh pushed a commit to branch master
> in repository https://gitbox.apache.org/repos/asf/incubator-ponymail-unit-tests.git
>
>
> The following commit(s) were added to refs/heads/master by this push:
> new 5793bdc Allow other versions to specifically make use of 'alternate'
> 5793bdc is described below
>
> commit 5793bdc5d3c3af9fb6d1ce0378bfd8c319a17b88
> Author: Daniel Gruno <hu...@apache.org>
> AuthorDate: Wed Jun 2 19:27:40 2021 +0200
>
> Allow other versions to specifically make use of 'alternate'
>
> Foal needs to supply an alternate in some places due to format=flowed
> changes. This probably warrants a discussion on the mailing list.
> ---
> tests/test-generators.py | 2 +-
> tests/test-parsing.py | 5 ++++-
> yaml/generate-httpd-users-200606-part.yaml | 2 ++
> yaml/gens-asterix-dev-201709-mailarchivesao.yaml | 1 +
> yaml/pars-asterix-dev-201709-mailarchivesao.yaml | 2 ++
> yaml/parsing-httpd-users-200606-part.yaml | 2 ++
> 6 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/tests/test-generators.py b/tests/test-generators.py
> index d730b3d..52b03d1 100755
> --- a/tests/test-generators.py
> +++ b/tests/test-generators.py
> @@ -148,7 +148,7 @@ def run_tests(args):
>
> expected = test['generated']
> alternate = expected
> - if archie.version == '10':
> + if archie.version == '10' or archie.version == _env.get('ALT_VERSION'):
> alternate = test.get('alternate') # alternate value for v0.10
> actual = json['mid']
> if actual != expected and actual != alternate:
> diff --git a/tests/test-parsing.py b/tests/test-parsing.py
> index fa55c86..f0ede42 100755
> --- a/tests/test-parsing.py
> +++ b/tests/test-parsing.py
> @@ -81,6 +81,9 @@ def run_tests(args):
>
> test_args = collections.namedtuple('testargs', ['parse_html'])(parse_html)
> archie = interfacer.Archiver(archiver, test_args)
> + _env = {}
> + if 'args' in yml and 'env' in yml['args']:
> + _env = yml['args']['env']
>
> mboxfiles = []
>
> @@ -114,7 +117,7 @@ def run_tests(args):
> body_sha3_256 = hashlib.sha3_256(json['body'].encode('utf-8')).hexdigest()
> expected = test['body_sha3_256']
> alternate = expected
> - if archie.version == '10':
> + if archie.version == '10' or archie.version == _env.get('ALT_VERSION'):
> alternate = test.get('alternate') # alternate value for v0.10
> if body_sha3_256 != expected and body_sha3_256 != alternate:
> errors += 1
> diff --git a/yaml/generate-httpd-users-200606-part.yaml b/yaml/generate-httpd-users-200606-part.yaml
> index a3c4408..3b7055a 100644
> --- a/yaml/generate-httpd-users-200606-part.yaml
> +++ b/yaml/generate-httpd-users-200606-part.yaml
> @@ -3,6 +3,8 @@
> args:
> cmd: tests/test-generators.py --generate generate-httpd-users-200606-part.yaml --mbox corpus/httpd-users-200606-part.mbox
> --rootdir ../ponymail/
> + env:
> + ALT_VERSION: foal
> generators:
> corpus/httpd-users-200606-part.mbox:
> cluster:
> diff --git a/yaml/gens-asterix-dev-201709-mailarchivesao.yaml b/yaml/gens-asterix-dev-201709-mailarchivesao.yaml
> index d97437f..67fe530 100644
> --- a/yaml/gens-asterix-dev-201709-mailarchivesao.yaml
> +++ b/yaml/gens-asterix-dev-201709-mailarchivesao.yaml
> @@ -4,6 +4,7 @@ args:
> --generate yaml/gens-asterix-dev-201709-mailarchivesao.yaml --root ../ponymail
> env:
> MOCK_GMTIME: "0"
> + ALT_VERSION: foal
> generators:
> corpus/asterix-dev-201709-mailarchivesao.mbox:
> cluster:
> diff --git a/yaml/pars-asterix-dev-201709-mailarchivesao.yaml b/yaml/pars-asterix-dev-201709-mailarchivesao.yaml
> index cea616f..a91005b 100644
> --- a/yaml/pars-asterix-dev-201709-mailarchivesao.yaml
> +++ b/yaml/pars-asterix-dev-201709-mailarchivesao.yaml
> @@ -2,6 +2,8 @@ args:
> cmd: tests/test-parsing.py --mbox corpus/asterix-dev-201709-mailarchivesao.mbox
> --generate yaml/pars-asterix-dev-201709-mailarchivesao.yaml --root ../ponymail
> parse_html: false
> + env:
> + ALT_VERSION: foal
> parsing:
> corpus/asterix-dev-201709-mailarchivesao.mbox:
> - index: 0
> diff --git a/yaml/parsing-httpd-users-200606-part.yaml b/yaml/parsing-httpd-users-200606-part.yaml
> index 4ff5bb4..d55d02b 100644
> --- a/yaml/parsing-httpd-users-200606-part.yaml
> +++ b/yaml/parsing-httpd-users-200606-part.yaml
> @@ -4,6 +4,8 @@ args:
> cmd: tests/test-parsing.py --generate parsing-httpd-users-200606-part.yaml --mbox corpus/httpd-users-200606-part.mbox
> --rootdir ../ponymail/
> parse_html: false
> + env:
> + ALT_VERSION: foal
> parsing:
> corpus/httpd-users-200606-part.mbox:
> - index: 0
>
Re: [incubator-ponymail-unit-tests] branch master updated: Allow
other versions to specifically make use of 'alternate'
Posted by sebb <se...@gmail.com>.
On Thu, 3 Jun 2021 at 09:21, Daniel Gruno <hu...@apache.org> wrote:
>
> On 03/06/2021 00.21, sebb wrote:
> > On Wed, 2 Jun 2021 at 18:32, Daniel Gruno <hu...@apache.org> wrote:
> >>
> >> I've made it so that the tests will pass with 56 specific format=flowed
> >> issues in mind.
> >
> > Wrong, so wrong.
>
> Depends on what the goal of the test is. We seem to disagree on what
> they are there for, and that's fair. I'm not hung up on pony and foal
> being exactly equal in their results,
Huh? That's precisely the issue here.
> I use the tests to guard against
> unintended changes. When the change is very much intended, I expect the
> tests to allow for that.
I agree that if a change is expected, it should be allowed for.
That is what was done for v0.10, as there is no way to change the past.
But it does not apply here.
A re-implementation of an existing generator MUST produce the same
output for the same input.
Otherwise it is broken.
It's unfortunate that there were no unit tests until recently,
otherwise the changes to the output would have been noticed before
they could cause any issues.
> >
> >> We have the option of:
> >> - Be strict and let foal fail forever
> >> - Be flexible and have foal and pony differ in these specific cases
> >> - Create a separate set of tests for both
> >
> > Or, fix foal so it generates the same results for the same generators.
>
> Fixing here would be "fixing" by breaking improvements - I am not
> willing to do that. I'd rather we yank out the legacy, medium and
> cluster generators and then get sean's DKIM patches merged in and settle
> on DKIM and full as the new standard generators.
A generator is not 'improved' by changing the code in such a way as to
alter the output.
It is either a bug, or a different generator.
The output of the generator for a given input is a fundamental part of
its public API.
SHA1 is an improvement over MD5, but you cannot 'improve' an MD5
generator by producing a SHA1 hash instead.
Note that the FULL generator must continue to generate the same output
as previously.
The DKIM generator is not thus encumbered currently, as it has not
been released (I hope).
> I'll look at those patches as time allows today.
>
> This won't solve the parsing tests however, where the two versions
> produce different results due to how they parse format=flowed and how
> some character encodings are handled. Here we either need to keep the
> alternate flag, or split it into tests for pony and tests for foal, and
> have runall figure that out somehow.
I agree that the parsing tests are different.
Most output should be unaffected, but there will be cases where output
can be improved.
In such cases, it would be better to have a version-specific attribute.
Otherwise it could hide changes where they are not expected.
For example, a change to Foal that accidentally reverts a fix so the
old output is generated.
That would be a bug, but it would not be detected.
> >
> > I realise that foal has deprecated the old generators, however it
> > still offers them as options.
> >
> > There is no point in doing so if the results are different, which is
> > why the tests were set up.
> > Fixing the tests does not make the code correct, it just hides the problems.
> >
> > We have to make allowances for v 0.10, because incompatible changes
> > were made previously.
> > It's not possible to change the past, so we have to live with it.
> >
> > However, for new code, it is possible to get it right -- or at least
> > not make it more wrong.
> >
> >> I am leaning towards option 2, which is how it is right now. Option 1
> >> means we can't use the tests for foal, and option 3 would mean we risk
> >> not noticing when the two diverge.
> >>
> >> For the places where the tests diverge (and need alternates), I've put a
> >> top comment in each YAML specifying who/why. I think this is currently
> >> the best compromise.
> >
> > There is no need to compromise.
> >
> > Foal needs to generate the same output for the same input for all the
> > historic generators that it supports.
> >
> > Anything else just makes an already bad situation worse.
> >
> >>
>
Re: [incubator-ponymail-unit-tests] branch master updated: Allow
other versions to specifically make use of 'alternate'
Posted by Daniel Gruno <hu...@apache.org>.
On 03/06/2021 00.21, sebb wrote:
> On Wed, 2 Jun 2021 at 18:32, Daniel Gruno <hu...@apache.org> wrote:
>>
>> I've made it so that the tests will pass with 56 specific format=flowed
>> issues in mind.
>
> Wrong, so wrong.
Depends on what the goal of the test is. We seem to disagree on what
they are there for, and that's fair. I'm not hung up on pony and foal
being exactly equal in their results, I use the tests to guard against
unintended changes. When the change is very much intended, I expect the
tests to allow for that.
>
>> We have the option of:
>> - Be strict and let foal fail forever
>> - Be flexible and have foal and pony differ in these specific cases
>> - Create a separate set of tests for both
>
> Or, fix foal so it generates the same results for the same generators.
Fixing here would be "fixing" by breaking improvements - I am not
willing to do that. I'd rather we yank out the legacy, medium and
cluster generators and then get sean's DKIM patches merged in and settle
on DKIM and full as the new standard generators.
I'll look at those patches as time allows today.
This won't solve the parsing tests however, where the two versions
produce different results due to how they parse format=flowed and how
some character encodings are handled. Here we either need to keep the
alternate flag, or split it into tests for pony and tests for foal, and
have runall figure that out somehow.
>
> I realise that foal has deprecated the old generators, however it
> still offers them as options.
>
> There is no point in doing so if the results are different, which is
> why the tests were set up.
> Fixing the tests does not make the code correct, it just hides the problems.
>
> We have to make allowances for v 0.10, because incompatible changes
> were made previously.
> It's not possible to change the past, so we have to live with it.
>
> However, for new code, it is possible to get it right -- or at least
> not make it more wrong.
>
>> I am leaning towards option 2, which is how it is right now. Option 1
>> means we can't use the tests for foal, and option 3 would mean we risk
>> not noticing when the two diverge.
>>
>> For the places where the tests diverge (and need alternates), I've put a
>> top comment in each YAML specifying who/why. I think this is currently
>> the best compromise.
>
> There is no need to compromise.
>
> Foal needs to generate the same output for the same input for all the
> historic generators that it supports.
>
> Anything else just makes an already bad situation worse.
>
>>
Re: [incubator-ponymail-unit-tests] branch master updated: Allow
other versions to specifically make use of 'alternate'
Posted by sebb <se...@gmail.com>.
On Wed, 2 Jun 2021 at 18:32, Daniel Gruno <hu...@apache.org> wrote:
>
> I've made it so that the tests will pass with 56 specific format=flowed
> issues in mind.
Wrong, so wrong.
> We have the option of:
> - Be strict and let foal fail forever
> - Be flexible and have foal and pony differ in these specific cases
> - Create a separate set of tests for both
Or, fix foal so it generates the same results for the same generators.
I realise that foal has deprecated the old generators, however it
still offers them as options.
There is no point in doing so if the results are different, which is
why the tests were set up.
Fixing the tests does not make the code correct, it just hides the problems.
We have to make allowances for v 0.10, because incompatible changes
were made previously.
It's not possible to change the past, so we have to live with it.
However, for new code, it is possible to get it right -- or at least
not make it more wrong.
> I am leaning towards option 2, which is how it is right now. Option 1
> means we can't use the tests for foal, and option 3 would mean we risk
> not noticing when the two diverge.
>
> For the places where the tests diverge (and need alternates), I've put a
> top comment in each YAML specifying who/why. I think this is currently
> the best compromise.
There is no need to compromise.
Foal needs to generate the same output for the same input for all the
historic generators that it supports.
Anything else just makes an already bad situation worse.
>