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

Hex encode the data in dump test #1637

Merged
merged 2 commits into from
Jan 29, 2025
Merged

Conversation

madolson
Copy link
Member

@madolson madolson commented Jan 28, 2025

Addresses the failure here: https://github.com/valkey-io/valkey/actions/runs/13000845302/job/36259016156#step:5:7272.

This change does three things:

  1. For some reason TCL 8.5 (which is used on macos) is handling \x03ba as \0xba, according to https://www.tcl-lang.org/man/tcl8.5/TclCmd/Tcl.htm#M27 so we encode "bar" using hex escapes too.
  2. Fix a spacing issue.
  3. Make it so that if the restore fails, it immediately errors.

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.86%. Comparing base (e9b8970) to head (7c06bc2).
Report is 7 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1637      +/-   ##
============================================
- Coverage     71.04%   70.86%   -0.18%     
============================================
  Files           121      121              
  Lines         65174    65176       +2     
============================================
- Hits          46304    46189     -115     
- Misses        18870    18987     +117     

see 15 files with indirect coverage changes

tests/unit/dump.tcl Show resolved Hide resolved
tests/unit/dump.tcl Show resolved Hide resolved
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Thanks! MacOS is weird...

The comments above the changed strings were supposed to point to the right parts inside the strings, to make it easier to read the strings.

tests/unit/dump.tcl Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

For some reason macos is not properly handling \x03ba. I don't know why, and couldn't find any existing issue for it. Replacing it with it's actual

I think I found it. MacOS is running TCL 8.5, right? https://www.tcl-lang.org/man/tcl8.5/TclCmd/Tcl.htm#M27

\xhh
The hexadecimal digits hh give an eight-bit hexadecimal value for the Unicode character that will be inserted. Any number of hexadecimal digits may be present; however, all but the last two are ignored (the result is always a one-byte quantity). The upper bits of the Unicode character will be 0.

This seems to have change in TCL 8.6 https://www.tcl-lang.org/man/tcl8.6/TclCmd/Tcl.htm#M27 to

\xhh
The hexadecimal digits hh (one or two of them) give an eight-bit hexadecimal value for the Unicode character that will be inserted. The upper bits of the Unicode character will be 0 (i.e., the character will be in the range U+000000–U+0000FF).

Comment on lines 139 to +141
r config set rdb-version-check strict
assert_equal {bar} [r get foo]
set e
} {OK}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I used catch instead of letting it fail immediately was to restore the config afterwards so even if this test case fails, it doesn't affect other test cases. Maybe that's too fancy.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Since you're traveling, I'll apply the suggestions and merge it so we can fix daily.

tests/unit/dump.tcl Outdated Show resolved Hide resolved
tests/unit/dump.tcl Show resolved Hide resolved
Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast changed the title Hex encode all of the data in dump test Hex encode the data in dump test Jan 29, 2025
@hwware hwware merged commit d3aabd7 into valkey-io:unstable Jan 29, 2025
49 checks passed
enjoy-binbin pushed a commit to enjoy-binbin/valkey that referenced this pull request Feb 2, 2025
Addresses the failure here:
https://github.com/valkey-io/valkey/actions/runs/13000845302/job/36259016156#step:5:7272.

This change does three things:
1. For some reason TCL 8.5 (which is used on macos) is handling `\x03ba`
as `\0xba`, according to
https://www.tcl-lang.org/man/tcl8.5/TclCmd/Tcl.htm#M27 so we encode
"bar" using hex escapes too.
2. Fix a spacing issue. 
3. Make it so that if the restore fails, it immediately errors.

---------

Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
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