Skip to content
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

Casenorm tests: enable backslash interpretation #8812

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

gmelikov
Copy link
Member

echo must have -e argument to properly
interpret unusual symbols.

Signed-off-by: George Melikov mail@gmelikov.ru

Motivation and Context

Fix #7633

Description

Thanks to @ikozhukhov , the problem resolves with this patch.

How Has This Been Tested?

WIP, waiting for buildbot successfull finish.

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:

Copy link
Contributor

@ikozhukhov ikozhukhov left a comment

Choose a reason for hiding this comment

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

experiment with printf will be interest

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label May 24, 2019
@gmelikov
Copy link
Member Author

Ah, the problem is with non-existing zlook at Linux, will try to replace it.

@behlendorf
Copy link
Contributor

Thanks for looking in to this. As part of the final fix you're going to want to remove the casenorm exceptions from tests/test-runner/bin/zts-report.py too. Also if you'd like to iterate more quickly with the CI on this, I'd suggest adding TEST_ZFSTESTS_TAGS="casenorm" to your commit message so only that test group is run.

@codecov
Copy link

codecov bot commented May 25, 2019

Codecov Report

Merging #8812 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8812      +/-   ##
==========================================
+ Coverage   79.11%   79.14%   +0.03%     
==========================================
  Files         419      419              
  Lines      123704   123696       -8     
==========================================
+ Hits        97870    97902      +32     
+ Misses      25834    25794      -40
Flag Coverage Δ
#kernel 79.9% <ø> (+0.18%) ⬆️
#user 66.46% <ø> (-0.47%) ⬇️

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 8221bcf...08ebb5c. Read the comment docs.

@rlaager
Copy link
Member

rlaager commented May 27, 2019

echo -e is not POSIX compliant. Starting from bash:

$ echo -e 'F\0303\0257L\0303\0253N\0303\0204m\0303\0253'
FïLëNÄmë
$ dash
$ echo -e 'F\0303\0257L\0303\0253N\0303\0204m\0303\0253'
-e FïLëNÄmë

Is this guaranteed to run under bash?

@gmelikov gmelikov force-pushed the casenorm branch 3 times, most recently from 653962e to c28530e Compare June 2, 2019 14:46
@gmelikov
Copy link
Member Author

gmelikov commented Jun 3, 2019

@ikozhukhov @rlaager I've got it working via printf, just replaced lines with needed format for printf.

@behlendorf thank you, removed tests from exclusion list.

Used test -f as a cheap replacement for zlook (maybe it's not the best path). Got a problem with some tests, will dig on weekend.

@PrivatePuffin
Copy link
Contributor

@gmelikov looks like a simple (partial) fix, any news on this? Being WIP and such...

@gmelikov
Copy link
Member Author

@Ornias1993 unfortunately it's not so simple - printf does work in Linux, but some tests are failing, I think there are some problems with ksh in Linux, but I didn't find the root cause yet.

I've updated PR and excluded problematic tests from PR for now, let's see tests and maybe merge this change as is.

@gmelikov gmelikov changed the title [WIP] Casenorm tests: enable backslash interpretation Casenorm tests: enable backslash interpretation Nov 15, 2019
@gmelikov gmelikov added Status: Work in Progress Not yet ready for general review and removed Status: Work in Progress Not yet ready for general review labels Nov 15, 2019
@behlendorf
Copy link
Contributor

This could be an issue with ksh-2020.0.0, or perhaps an intentional change in behavior. When running ksh interactively it correctly interprets the unicode, but for some reason when invoked via the test suite not all \uHHHH values are interpreted (i.e \u0308).

ksh --version
  version         sh (AT&T Research) 2020.0.0
[behlendo@localhost zfs]$ ksh
$ printf '\u0066\u0069\u0308\u006c\u0065\u0308\u006e\u0061\u0308\u006d\u0065\u0308'
fïlënämë$ 

For example, a snipet from the ZTS output:

printf '\u0066\u0069\u0308\u006c\u0065\u0308\u006e\u0061\u0308\u006d\u0065\u0308'
fi\u0308le\u0308na\u0308me\u0308

Using bash -c "-printf '...'" as a workaround worked for me on Fedora 31. It's pretty ugly, but we could probably live with it as a Linux-only change. When applied all of the casenorm tests pass.

NAME_C_ORIG=$(bash -c "printf '\u0046\u00ef\u004c\u00eb\u004e\u00c4\u006d\u00eb'")
NAME_C_UPPER=$(bash -c "printf '\u0046\u00cf\u004c\u00cb\u004e\u00c4\u004d\u00cb'")
NAME_C_LOWER=$(bash -c "printf '\u0066\u00ef\u006c\u00eb\u006e\u00e4\u006d\u00eb'")
NAME_D_ORIG=$(bash -c "printf '\u0046\u0069\u0308\u004c\u0065\u0308\u004e\u0041\u0308\u006d\u0065\u0308'")
NAME_D_UPPER=$(bash -c "printf '\u0046\u0049\u0308\u004c\u0045\u0308\u004e\u0041\u0308\u004d\u0045\u0308'")
NAME_D_LOWER=$(bash -c "printf '\u0066\u0069\u0308\u006c\u0065\u0308\u006e\u0061\u0308\u006d\u0065\u0308'")

@behlendorf
Copy link
Contributor

Ahh, it seems the locale is being lost from the environment. @gmelikov adding the following hunk to this PR should be enough.

diff --git a/tests/zfs-tests/tests/functional/casenorm/casenorm.cfg b/tests/zfs-tests/tests/functional/casenorm/casenorm.cfg
index e35a726e7df..f955d33cd19 100644
--- a/tests/zfs-tests/tests/functional/casenorm/casenorm.cfg
+++ b/tests/zfs-tests/tests/functional/casenorm/casenorm.cfg
@@ -17,6 +17,9 @@
 # Copyright (c) 2016 by Delphix. All rights reserved.
 #
 
+LANG=en_US.UTF-8
+LC_ALL=en_US.UTF-8
+
 NAME_C_ORIG=$(printf '\u0046\u00ef\u004c\u00eb\u004e\u00c4\u006d\u00eb')
 NAME_C_UPPER=$(printf '\u0046\u00cf\u004c\u00cb\u004e\u00c4\u004d\u00cb')
 NAME_C_LOWER=$(printf '\u0066\u00ef\u006c\u00eb\u006e\u00e4\u006d\u00eb')

@rlaager
Copy link
Member

rlaager commented Nov 16, 2019

Instead of "en_US", does "C" or "C.UTF-8" work? If so, it might be better to use that. Otherwise, en_US.UTF-8 is probably fine.

@gmelikov gmelikov force-pushed the casenorm branch 4 times, most recently from e7da159 to d6d3ebb Compare November 17, 2019 15:40
@gmelikov
Copy link
Member Author

@behlendorf thank you! Unfortunately, locale fix didn't work for buildbot, so I've used bash hack.

@gmelikov gmelikov removed the Status: Work in Progress Not yet ready for general review label Nov 17, 2019
@gmelikov gmelikov added the Status: Work in Progress Not yet ready for general review label Nov 17, 2019
@gmelikov
Copy link
Member Author

Bash hack didn't work either for buildbot. Interesting.

@behlendorf
Copy link
Contributor

Can you try the following. @rlaager's suggestion works for me locally.

export LANG="C.UTF-8"
export LC_ALL="C.UTF-8"

Note that with this change I still do see 6 local failures like with the original PR. But everything looks to be escaped properly so those failures will need to be inspected individually.

$ ./scripts/zfs-tests.sh -T casenorm
...
Results Summary
PASS	  13
FAIL	   6

Running Time:	00:00:45
Percent passed:	68.4%
Log directory:	/var/tmp/test_results/20191118T093144

Tests with results other than PASS that are expected:
    FAIL casenorm/mixed_formd_delete (https://github.com/zfsonlinux/zfs/issues/7633)
    FAIL casenorm/mixed_formd_lookup (https://github.com/zfsonlinux/zfs/issues/7633)
    FAIL casenorm/mixed_formd_lookup_ci (https://github.com/zfsonlinux/zfs/issues/7633)
    FAIL casenorm/mixed_none_lookup_ci (https://github.com/zfsonlinux/zfs/issues/7633)
    FAIL casenorm/sensitive_formd_delete (https://github.com/zfsonlinux/zfs/issues/7633)
    FAIL casenorm/sensitive_formd_lookup (https://github.com/zfsonlinux/zfs/issues/7633)

Use `printf` to properly
interpret unusual symbols.

+ zlook doesn't exists in ZoL, use `test`.

Signed-off-by: George Melikov <mail@gmelikov.ru>
@gmelikov
Copy link
Member Author

@behlendorf ah, I thought you had all tests passed.

Added locale fix for fedora, excluded problematic tests. We may merge this PR and tackle with other tests next time.

@behlendorf behlendorf merged commit 540312d into openzfs:master Nov 20, 2019
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Work in Progress Not yet ready for general review labels Nov 20, 2019
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 26, 2019
Use `printf` to properly interpret unicode characters.

Illumos uses a utility called `zlook` to allow additional flags to be
provided to readdir and lookup for testing.  This functionality could
be ported to Linux, but even without it several of the tests can be
enabled by instead using the standard `test` command.

Additional, work is required to enable the remaining test cases.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: George Melikov <mail@gmelikov.ru>
Issue openzfs#7633
Closes openzfs#8812
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 27, 2019
Use `printf` to properly interpret unicode characters.

Illumos uses a utility called `zlook` to allow additional flags to be
provided to readdir and lookup for testing.  This functionality could
be ported to Linux, but even without it several of the tests can be
enabled by instead using the standard `test` command.

Additional, work is required to enable the remaining test cases.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: George Melikov <mail@gmelikov.ru>
Issue openzfs#7633
Closes openzfs#8812
tonyhutter pushed a commit that referenced this pull request Jan 23, 2020
Use `printf` to properly interpret unicode characters.

Illumos uses a utility called `zlook` to allow additional flags to be
provided to readdir and lookup for testing.  This functionality could
be ported to Linux, but even without it several of the tests can be
enabled by instead using the standard `test` command.

Additional, work is required to enable the remaining test cases.

Reviewed-by: Igor Kozhukhov <igor@dilos.org>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: George Melikov <mail@gmelikov.ru>
Issue #7633
Closes #8812
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.

Test case: casenorm test group
5 participants