[BUG] Error logging not treated as error by tests
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 15.7k
- Forks
- 5.6k
- Avg merge
- 2d 44m
- Merged PRs (30d)
- 80
Description
Description
This isn't really one particular issue, but rather a pattern of failures I've seen crop up again and again. Users seem to often run state.apply from the minion side, ie with salt-call. This could be a manual run or from cron, but the main point is that from the minion side this lets you see the logs from salt. With the default --log-level=warning, anything that warns or errors will be very visible. If run from cron, this will trigger an error email that the job failed since it produced output. However, the test suite generally does not consider error logging a problem, thus it doesn't catch a lot of regressions that might not actually break a given state, but causes lots of grief for users that now get errors flagged from all their systems since they suddenly started logging errors and triggering alerts all around. There's been lots of examples of this, see every PR that is fixed with output_loglevel='quiet' or ignore_retcode=True added like for example https://github.com/saltstack/salt/pull/57061. Having a state start error logging is a regression that should be caught by the test suite.
I'm not sure what the solution is, but I would see if it's possible to default to treating anything logged to warning or higher during a test run as an error, unless a specific test marks it as expected.
Somewhat similar is the problem of users seeing lots of deprecation warnings when running salt-call that is also not caught by the test suite.
Setup
Example setup in https://github.com/saltstack/salt/issues/54817 where a regression in git.latest breaks salt for users but tests didn't catch the problem.
Expected behavior
state.apply should never log with warning or higher if all states were applied successfully. Salt should detect these regressions before users do.
Additional context
Just some other examples of this:
https://github.com/saltstack/salt/pull/54998
https://github.com/saltstack/salt/pull/53911
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the test suite paths that exercise state.apply and salt-call, then reproduce the git.latest regression described in issue 54817. Determine how warning-or-higher logs are currently handled during tests and how expected warnings could be marked. Done means unexpected warning or error logs fail the test while explicitly expected logging remains allowed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100