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

Fix test failure on Mac #2816

Merged
merged 1 commit into from
Nov 4, 2023
Merged

Fix test failure on Mac #2816

merged 1 commit into from
Nov 4, 2023

Conversation

kevinbackhouse
Copy link
Collaborator

Fix this error, which is happening on Mac:

======================================================================
FAIL: test_run (github.test_issue_2427.issue_2427_BmffImage_brotliUncompress_memleak.test_run)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/exiv2/exiv2/tests/system_tests.py", line 652, in test_run
    self.compare_stderr(i, command, processed_stderr, stderr)
  File "/Users/runner/work/exiv2/exiv2/tests/system_tests.py", line 773, in compare_stderr
    self._compare_output(
  File "/Users/runner/work/exiv2/exiv2/tests/system_tests.py", line 745, in _compare_output
    self.assertMultiLineEqual(
AssertionError: 'Exiv[61 chars]/exiv2/test/data/issue_2427_poc.jpg:\n_ERROR_FORMAT_CL_SPACE\n' != 'Exiv[61 chars]/exiv2/test/data/issue_2427_poc.jpg:\nCL_SPACE\n'
  Exiv2 exception in print action for file /Users/runner/work/exiv2/exiv2/test/data/issue_2427_poc.jpg:
- _ERROR_FORMAT_CL_SPACE
+ CL_SPACE
 : Standard error does not match

git grep shows that the string CL_SPACE isn't coming from our own codebase, so it's probably coming from brotli. On Mac it's _ERROR_FORMAT_CL_SPACE instead.

@kevinbackhouse kevinbackhouse added this to the v0.28.1 milestone Nov 4, 2023
@ghost
Copy link

ghost commented Nov 4, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@kevinbackhouse kevinbackhouse added the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label Nov 4, 2023
Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #2816 (8099ed0) into 0.28.x (414bff1) will increase coverage by 0.05%.
Report is 62 commits behind head on 0.28.x.
The diff coverage is 51.26%.

@@            Coverage Diff             @@
##           0.28.x    #2816      +/-   ##
==========================================
+ Coverage   63.92%   63.98%   +0.05%     
==========================================
  Files         103      103              
  Lines       22306    22338      +32     
  Branches    10796    10821      +25     
==========================================
+ Hits        14259    14292      +33     
+ Misses       5827     5824       -3     
- Partials     2220     2222       +2     
Files Coverage Δ
include/exiv2/image.hpp 100.00% <ø> (ø)
include/exiv2/matroskavideo.hpp 60.00% <ø> (ø)
include/exiv2/slice.hpp 90.56% <ø> (ø)
include/exiv2/value.hpp 85.11% <100.00%> (ø)
src/canonmn_int.cpp 72.88% <100.00%> (ø)
src/futils.cpp 73.09% <ø> (ø)
src/minoltamn_int.cpp 66.52% <100.00%> (ø)
src/panasonicmn_int.cpp 52.58% <ø> (ø)
src/pentaxmn_int.cpp 71.36% <100.00%> (+0.85%) ⬆️
src/riffvideo.cpp 62.18% <ø> (ø)
... and 16 more

@neheb
Copy link
Collaborator

neheb commented Nov 4, 2023

It's different behavior from newer brotli, not just macOS.

@kevinbackhouse kevinbackhouse requested a review from neheb November 4, 2023 21:31
@neheb
Copy link
Collaborator

neheb commented Nov 4, 2023

why just 0.28.x? main is impacted too.

@kevinbackhouse
Copy link
Collaborator Author

why just 0.28.x? main is impacted too.

@neheb: I think the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label should handle that.

@neheb neheb merged commit ad8b806 into Exiv2:0.28.x Nov 4, 2023
102 checks passed
@neheb
Copy link
Collaborator

neheb commented Nov 4, 2023

TIL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
forward-to-main Forward changes in a 0.28.x PR to main with Mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants