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/08 01:46:01 UTC
[Bug 7988] New: Fix and streamline tests for release process
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
Bug ID: 7988
Summary: Fix and streamline tests for release process
Product: Spamassassin
Version: 4.0.0
Hardware: All
OS: All
Status: NEW
Severity: normal
Priority: P2
Component: Regression Tests
Assignee: dev@spamassassin.apache.org
Reporter: sidney@sidney.com
Target Milestone: Undefined
In preparing the 4.0.0 release I have come across some bugs and some
difficulties in running the tests in t and xt directories. I'm opening this
issue to fix all of them at once, but here is a list that can be discussed as
separate items:
1. The xt directory does not fulfill its purpose and is hard to maintain.
The commit message for its creation says that it is supposed to ensure that the
release manager runs all the tests. This is because the default settings in
t/config.dist skip certain tests if you just run make test. The intention is
that the release manager can run make test TEST_FILES="xt/*.t" and cause all
tests in t/*.t to be run without skipping the tests the are disabled by
default.
To implement this, each test in xt/*.t calls its corresponding test in t/ with
a flag override if the test is one that depends on a flag in t/config.dist
One problem with this is that it is hard to maintain. Every time a new test is
added in t/ the corresponding test has to be added to xt/. Also, some tests,
such as dcc.t depend on two flags, and it is easy to introduce a bug by not
overriding all the necessary flags when writing the test in xt/.
I propose to write a shell script named something like xt/release_test_suite.sh
that runs all files it finds in t/*.t with all the suitable option flags
enabled. This will serve the purpose of the xt directory without the
maintainability problems. All the small xt/*.t files that just call a file in
t/*.t can be deleted,
This isn't portable to Windows, but the target user of the script means it
doesn't have to be for now. It doesn't remove the ability of anyone running
Windows to run a test that they can now.
2. There are three test files in xt/ that are not mirrors of tests in t/*.t
These three are meant to be of interest only to release managers and packagers.
They would also be run by the new script.
3. There is some bit rot in the three xt-specific test scripts, both from
changes that have been made to t/SATest.pm and other code changes since 3.4
that were done with nobody running the xt tests. xt/20_saw_ampersand.t is
particularly bad because it can only be run in an ancient Perl version less
than 5.17, so it doesn't see much testing.
4. In testing the tests, I found that they don't work if run outside of using
make test, for example by running with the prove command. The problem results
from SATest.pm putting relative paths on @INC which then breaks if a test
changes directories. The setup of @INC in the Makefile for a test happens to
use the correct absolute pathname so running make test hides the bug in SATest.
Also, some tests are not calling SATest to initialize their environment, and
they only happen to work because Makefile sets up enough to work, which doesn't
happen if the test is run using prove.
5. There are 11 test files t/root*.t that are intended to be run using
something like sudo make test TEST_FILES="t/root*.t"
Currently they will only run if the uid is 0 (being run as root), and if a flag
in t/config.dist is set to enable the root tests.
I don't think the flag in config.dist is necessary. Nobody is likely to run the
root tests as root without intending to. I'm more concerned about someone who
doesn't realize they are in a root shell running all the non-root tests. I
would remove the run_root_tests flag from config.dist and add an optional
argument to sa_t_init() in SATest.pm that the t/root*.t tests can use to tell
sa_t_init that it is ok to run as root, having sa_t_init() otherwise fail if
running as root.
I already have fixes for 1-4 ready to commit. I can add 5 as a separate commit,
since that it pretty independent.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #6 from Kevin A. McGrail <km...@apache.org> ---
(In reply to Henrik Krohns from comment #5)
> (In reply to Sidney Markowitz from comment #4)
> > (In reply to Kevin A. McGrail from comment #1)
> > > Just one note that I believe the root flag was added back when the tests
> > > would possibly slam a production instance of something like spamd running on
> > > the same system on the same port.
> >
> > I'm open to the argument that having to set the run_root_tests flag in
> > config.dist or run using prove with a --override will be useful to ensure
> > that someone does not unthinkingly run sudo make test TEST_FILES="t/root*.t"
> > or even sudo make test on a production server.
> >
> > I personally wouldn't do it, but I'm not a sysadmin in a production
> > environment, so what do I know?
>
> Regardless of any flags, the tests should be made sure they won't affect any
> external spamd or write things outside of test directory. At quick glance,
> only spamd_prefork_stress* makes a dumb generic "pkill -f spamd child".
100% agreed and why I wanted to flag the issue with changing xt. Those tests
are run really only by RM's with and without privileged access. They are not
for the faint of heart to just run normally and could impact production systems
they way they are written now. There's been improvements especially around the
sudo tests but I documented the how I built a release box in the build/README
to try and help other RMs spin up something for testing.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
Henrik Krohns <ap...@hege.li> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |apache@hege.li
--- Comment #2 from Henrik Krohns <ap...@hege.li> ---
Good stuff. It should not only be easy for "release manager" to run all
possible tests, but all the release testers too.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
Kevin A. McGrail <km...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |kmcgrail@apache.org
--- Comment #1 from Kevin A. McGrail <km...@apache.org> ---
Just one note that I believe the root flag was added back when the tests would
possibly slam a production instance of something like spamd running on the same
system on the same port.
Totally support the idea of streamlining the release test procedure!
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #5 from Henrik Krohns <ap...@hege.li> ---
(In reply to Sidney Markowitz from comment #4)
> (In reply to Kevin A. McGrail from comment #1)
> > Just one note that I believe the root flag was added back when the tests
> > would possibly slam a production instance of something like spamd running on
> > the same system on the same port.
>
> I'm open to the argument that having to set the run_root_tests flag in
> config.dist or run using prove with a --override will be useful to ensure
> that someone does not unthinkingly run sudo make test TEST_FILES="t/root*.t"
> or even sudo make test on a production server.
>
> I personally wouldn't do it, but I'm not a sysadmin in a production
> environment, so what do I know?
Regardless of any flags, the tests should be made sure they won't affect any
external spamd or write things outside of test directory. At quick glance, only
spamd_prefork_stress* makes a dumb generic "pkill -f spamd child".
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #4 from Sidney Markowitz <si...@sidney.com> ---
(In reply to Kevin A. McGrail from comment #1)
> Just one note that I believe the root flag was added back when the tests
> would possibly slam a production instance of something like spamd running on
> the same system on the same port.
I'm open to the argument that having to set the run_root_tests flag in
config.dist or run using prove with a --override will be useful to ensure that
someone does not unthinkingly run sudo make test TEST_FILES="t/root*.t" or even
sudo make test on a production server.
I personally wouldn't do it, but I'm not a sysadmin in a production
environment, so what do I know?
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #3 from Kevin A. McGrail <km...@apache.org> ---
(In reply to Henrik Krohns from comment #2)
> Good stuff. It should not only be easy for "release manager" to run all
> possible tests, but all the release testers too.
That's feasible nowadays. The goal was that back in the dark ages of the
project, it took like 1 hr+ to run all the tests. And since all the tests are
supposed to be run before checking in code, we wanted to streamline it by using
extra tests.
Then we had some releases and check ins with so many problems we changed some
of the config.dist file to run more things. Long tests is what I remember
enabling it. That got us pack on a path of stability.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #7 from Sidney Markowitz <si...@sidney.com> ---
I'm also documenting more complete steps for a release box starting from a
minimal Ubuntu install, putting it on our wiki. Also, updating build/README on
svn.
Items 1-4 done. I'm going to think a bit about item 5 and maybe do another
commit about it before closing this.
Commit message:
bug 7988 Fixes and updates to regression tests
- All tests use common initialization in SATest.pm
- Use absolute pathname in @INC to fix breakage caused by chdir
- Some wording changes in test warnings
- Revamp xt tests to use one shell script that calls t/*.t and another 3 test
scripts
- Fix problems in saw-ampersand test and update for newer SpamAssassin code
Committed revision 1900676.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
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 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
Sidney Markowitz <si...@sidney.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #8 from Sidney Markowitz <si...@sidney.com> ---
I'm not going to remove the run_as_root flag in config.dist as part of this
issue. If I get motivated to do something like disable running all the non-root
tests from running as root, I'll open a new issue for it and reconsider the
flag as part of that.
--
You are receiving this mail because:
You are the assignee for the bug.
[Bug 7988] Fix and streamline tests for release process
Posted by bu...@spamassassin.apache.org.
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7988
--- Comment #9 from Sidney Markowitz <si...@sidney.com> ---
Adding another comment after having closed this issue:
After reading over bug #5529 which I was involved with and blessedly forgot
about, I see that 1) there are (or have been) people wanting to run make test
as root; and 2) I don't want to revisit the can of worms that turned out to be
and painstakingly got to work. So there are necessary and sufficient reasons
not to touch the way handle tests run as root and the separate t/root*.t tests,
including keeping the run_root_tests existing and defaulting to n.
--
You are receiving this mail because:
You are the assignee for the bug.