You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spamassassin.apache.org by bu...@spamassassin.apache.org on 2022/05/28 11:33:23 UTC

[Bug 7997] New: Test failures indicate problems in tests that need cleanup

https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

            Bug ID: 7997
           Summary: Test failures indicate problems in tests that need
                    cleanup
           Product: Spamassassin
           Version: SVN Trunk (Latest Devel Version)
          Hardware: All
                OS: All
            Status: NEW
          Severity: blocker
          Priority: P2
         Component: Regression Tests
          Assignee: dev@spamassassin.apache.org
          Reporter: sidney@sidney.com
  Target Milestone: Undefined

Some test failures that only occur when running without the full contents of
trunk/rules have revealed a number of problems and weaknesses in tests. For
example, t/rule_multiple.t contains test patterns
  q{ META_BODY_RULE }
  q{ META_BODY_RULE_MAX }

Resulting in these actual regex

\s*META_BODY_RULE\s*
\s*META_BODY_RULE_MAX\s*

which both match the string META_BODY_RULE_MAX

All the tests that have similar problems should be fixed even if they are not
currently failing, and SATest.pm should be changed to make it less likely that
tests will be written with these faults.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

Sidney Markowitz <si...@sidney.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Sidney Markowitz <si...@sidney.com> ---
trunk % svn ci -m "bug 7997 move non-rule settings from 01_test_rules.cf to
01_test_rules.pre"
Sending        t/data/01_test_rules.cf
Sending        t/data/01_test_rules.pre
Transmitting file data ..done
Committing transaction...
Committed revision 1901358.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

--- Comment #1 from Sidney Markowitz <si...@sidney.com> ---
committed by hege:

revision 1901346
Log:
Test cleanups and fixes.

Note that %patterns has now two exact patterns styles:

- Literal strings match exactly the string.  Whitespace is no longer ignored
  (any leading and trailing whitelist must match), but consecutive
  whitespace is normalized:

  q{ FOO } => ''
  ' FOO ' => ''

- Regular expressions, defined with standard qr// operator:

  qr/ FOO / => ''

revision 1901348
Log:
Don't clear any tstprefs() or tstlocalrules() settings with clear_localrules()

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

--- Comment #7 from Henrik Krohns <ap...@hege.li> ---
Looks fine to me..

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

Sidney Markowitz <si...@sidney.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #3 from Sidney Markowitz <si...@sidney.com> ---
t/rule_multiple.t is failing in make disttest, though works in make test

In email, hege said about it

> I can add the "report _SUMMARY_" to rule_multiple.t, then it works.

> Dunno what would be the most clear way to resolve this.  Obviously the
> report stuff is not "rules" per say, maybe it should even be in
> 01_test_rules.pre so it never gets removed.

As it says in 01_test_rules.cf

# Rules used in the test suite.  This allows us to change the
# main ruleset without breaking the test suite.

How about adding 01_test_rules.cf to the list of rule files in
clear_local_rules() that it doesn't clear?

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

Bill Cole <bi...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |billcole@apache.org

--- Comment #8 from Bill Cole <bi...@apache.org> ---
(In reply to Sidney Markowitz from comment #6)
> That passed the tests. If you agree with that approach, I'll commit it.

+1

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

Sidney Markowitz <si...@sidney.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sidney@sidney.com
   Target Milestone|Undefined                   |4.0.0

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

Henrik Krohns <ap...@hege.li> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |apache@hege.li
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #2 from Henrik Krohns <ap...@hege.li> ---
This was fixed with Revision 1901346.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

--- Comment #6 from Sidney Markowitz <si...@sidney.com> ---
That passed the tests. If you agree with that approach, I'll commit it.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

--- Comment #4 from Henrik Krohns <ap...@hege.li> ---
(In reply to Sidney Markowitz from comment #3)
>
> How about adding 01_test_rules.cf to the list of rule files in
> clear_local_rules() that it doesn't clear?

I haven't quite followed how tests are supposed to work after your changes.

So apparently when testing in trunk, ../rules/*.cf is still always copied to
localrules?

And disttest only pretty much has localrules/01_test_rules.cf?

If that's the case, sure, I guess it simplest to allow the few rules in
01_test_rules.cf remain.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 7997] Test failures indicate problems in tests that need cleanup

Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7997

--- Comment #5 from Sidney Markowitz <si...@sidney.com> ---
(In reply to Henrik Krohns from comment #4)
> If that's the case, sure, I guess it simplest to allow the few rules in
> 01_test_rules.cf remain.

Now that I think more about it, I am less sure.

I didn't want to make major changes, so I left it the way it was, which is

 make test runs in the t directory and copies ../rules/*.cf to localrules

 data/01_test_rules.cf contains rules that tests can assume are defined even if
make test is not run from trunk with all trunk/rules

 make disttest creates a subdirectory, copies MANIFEST files there (which
include just a subset of trunk/rules) and runs make and make test from there

 clear_localrules() can be called by tests that want to make sure that they
control what rules are defined.

I think that clear_localrules() should clear local rules, even those in
01_test_rules.cf because those are just an arbitrary subset that were being
used by tests. We can move these settings to 01_test_rules.pre because they are
settings, not rules. What do you think?

clear_report_template
report _SUMMARY_

clear_headers

add_header spam Flag _YESNOCAPS_
add_header all Level _STARS(*)_
add_header all Status "_YESNO_, score=_SCORE_ required=_REQD_ tests=_TESTS_
autolearn=_AUTOLEARN_ version=_VERSION_"

ifplugin Mail::SpamAssassin::Plugin::DCC
use_dcc 0
endif
ifplugin Mail::SpamAssassin::Plugin::Razor2
use_razor2 0
endif
ifplugin Mail::SpamAssassin::Plugin::Pyzor
use_pyzor 0
endif

-- 
You are receiving this mail because:
You are the assignee for the bug.