Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 19, 2019

Signed-off-by: Ryan Moeller ryan@ixsystems.com

Motivation and Context

The regex pattern to extract test results from the test runner was not updated to match when the test output changed to include a platform identifier for platform specific tests. This caused failing platform-specific tests to be missed.

Description

Add an optional non-capturing group to match the platform identifier.

How Has This Been Tested?

Manually ran zfs-test.sh for various tags to confirm correct operation:

  • xattr tests from linux.run, run without attr tool, some pass some fail, failed tests are caught
  • casenorm tests from common.run, some pass some fail, failed tests are caught

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Dec 19, 2019
@ghost ghost requested a review from behlendorf December 19, 2019 19:08
@behlendorf behlendorf requested a review from jwk404 December 19, 2019 19:11
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks for running this down. It'll be interesting to get a full test run to see what may have slipped in. We may need to hold off on this one briefly until things like #9748, and perhaps others, are merged.

@ghost ghost force-pushed the zts-report-id-fix branch from 336d3c1 to 7b72156 Compare December 19, 2019 19:25
@ghost
Copy link
Author

ghost commented Dec 19, 2019

  • Split pattern string across several lines for style

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 19, 2019
@behlendorf
Copy link
Contributor

behlendorf commented Dec 19, 2019

That's good, early results suggest that only the handful of tests from #9748 were impacted. Since those fixes are now merged, if you rebase this you should get a clean run.

Tests with results other than PASS that are unexpected:
    FAIL devices/devices_001_pos (expected PASS)
    FAIL devices/devices_002_neg (expected PASS)
    FAIL procfs/pool_state (expected PASS)

The pattern was not updated to match when the test output changed to
include a platform identifier for platform specfic tests.

Signed-off-by: Ryan Moeller <ryan@ixsystems.com>
@ghost ghost force-pushed the zts-report-id-fix branch from 7b72156 to 27df83d Compare December 20, 2019 02:06
@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #9750 into master will decrease coverage by <1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #9750    +/-   ##
========================================
- Coverage      80%      80%   -<1%     
========================================
  Files         385      385            
  Lines      121470   121470            
========================================
- Hits        96667    96640    -27     
- Misses      24803    24830    +27
Flag Coverage Δ
#kernel 80% <ø> (ø) ⬆️
#user 67% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fb2771...27df83d. Read the comment docs.

@behlendorf behlendorf merged commit 54aefa6 into openzfs:master Dec 20, 2019
@ghost ghost deleted the zts-report-id-fix branch December 20, 2019 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Accepted Ready to integrate (reviewed, tested)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants