Skip to content

ZTS: Fix two bugs in the zpool dry run tests #17198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

AttilaFueloep
Copy link
Contributor

Motivation and Context

The output changes in #17045 broke the zpool dry run tests. This PR fixes the failing tests.

Description

Rework the tests to not depend on log output format. While here, fix a bug where "\n" was printed literally, not as a line break as intended.

How Has This Been Tested?

I've run the modified tests manually forcing test failures by editing the tests accordingly.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for jumping on this. I was part-way through a very similar change late last night but I got tired and went to sleep instead 💤

@AttilaFueloep
Copy link
Contributor Author

Well, it's a matter of course, if I break things I should fix them. Sorry for the duplicate work.

@robn
Copy link
Member

robn commented Mar 30, 2025

Bit hard to claim you broke it five years later :) But I appreciate the diligence!

Incidentally, I've just been playing with this as maybe a useful library function:

function capture_output {
  typeset -n out=$1
  typeset cmd=$2
  shift 2
  out=$($cmd $*)
}

typeset out
log_must capture_output out zpool status

Seems to maybe solve both the need to capture output, and to ask for a test assert on the return value. But I'm just playing, haven't thought it through, and definitely not something this PR should block on I think.

@AttilaFueloep
Copy link
Contributor Author

AttilaFueloep commented Mar 30, 2025

I like this idea, very elegant. The question would be where to put it.

Adding a function which does what we need came to my mind as well, but I decided against it since it would just be used in four places. But that's something to think of tomorrow, it's bedtime in my timezone.

@AttilaFueloep
Copy link
Contributor Author

AttilaFueloep commented Apr 1, 2025

There is a function log_neg_expect in logapi.shlib which roughly does the opposite of what we would need here. So it would make sense to add something like log_pos_expect, especially since I've a PR in the pipe which also checks for expected output of cli tools. Before doing that I'd prefer to have a better understanding of the inner workings of the test-runner though.

Since this PR has already two reviews (thanks!) I'd prefer not to delay merging and to follow-up later.

@tonyhutter
Copy link
Contributor

Merged as:
5b0c27c ZTS: Fix zpool dry run tests output formating
029c4ae ZTS: Fix zpool dry run tests depending on output format

@tonyhutter tonyhutter closed this Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants