- 
                Notifications
    
You must be signed in to change notification settings  - Fork 10.5k
 
Fix ResponseCompression tests to work with different zlib implementations #63988
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 ResponseCompression tests to work with different zlib implementations #63988
Conversation
| 
           Thanks a lot for the contribution @medhatiwari! This is a great change and allows us to run our tests against multiple zlib implementations! Ideally we'd want to run the tests in CI against both of those to have coverage and detect regressions but this is a good first step. The tradeoff here of losing brittleness for flexibility feels worth it, especially due to the improvement in correctness coverage. cc: @BrennanConroy @akoeplinger any other thoughts here or can we go ahead and merge this in?  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general idea is good.
        
          
                src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                src/Middleware/ResponseCompression/test/ResponseCompressionMiddlewareTest.cs
          
            Show resolved
            Hide resolved
        
      | 
           Can this be merged , if there are no more code changes needed ?  | 
    
| [InlineData(new[] { "identity;q=0.5", "gzip" }, 29)] | ||
| public async Task Request_AcceptWithHigherCompressionQuality_Compressed(string[] acceptEncodings, int expectedBodyLength) | ||
| { | ||
| _ = expectedBodyLength; // Parameter kept for test data compatibility | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think we care about preserving the test data from past runs for comparison here. Can just remove this param from where it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed all the unused expectedBodyLength parameters and discard assignments
Fix ResponseCompression tests for system zlib compatibility
Fixes #63969
Changes
CheckResponseCompressedCheckCompressionFunctionalitymethod to verify compression by decompressing contentTesting
All existing tests pass with both zlib-ng and system zlib implementations.
cc: @giritrivedi