Skip to content

Commit c7e4932

Browse files
authored
test: make tests independent of zip encoding (#3239)
### Description Extracted from #3237 This change was triggered by bumping `symbolic` to `12.17.3` to include getsentry/symbolic#960. `symbolic` updated its `zip` dependency (`2.4.2` to `7.2.0`) since the last bump. This changed encoding internals and invalidated a bunch of test assertions. The PR addresses the issue in the following way: I rewrote the affected tests so that they decode the incoming chunks and compare their contents against the fixtures, rather than snapshotting, which could break with every change to the encoder. I also simplified the usage of `split_chunk_body()` a bit: now it returns a `Vec`, and callers decide how they want to view the data: the simple tests collect into a `HashSet` for set comparison, while the small-chunk tests use a `SHA1` digest multimap to verify exact chunk identity and multiplicity. If you actually wanted an early warning whether the encoding was stable, then we can drop this PR. Specifically, for `build_deterministic()` in `src/utils/source_bundle.rs`, it was not entirely clear whether you want two runs of the same version to have predictably the same output (which I changed it to) or whether it should actually be stable across future versions (which a `zip` crate bump upstream would break). ### Issues Raised as part of the fix proposals for getsentry/sentry#104738 ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.
1 parent f825541 commit c7e4932

18 files changed

Lines changed: 488 additions & 382 deletions

src/utils/source_bundle.rs

Lines changed: 58 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -195,13 +195,41 @@ fn url_to_bundle_path(url: &str) -> Result<String> {
195195

196196
#[cfg(test)]
197197
mod tests {
198-
use sha1_smol::Sha1;
199198
use symbolic::debuginfo::sourcebundle::SourceFileType;
200199

201200
use crate::utils::file_upload::SourceFile;
202201

203202
use super::*;
204203

204+
/// Build a source bundle from the standard pair of sourcemap fixtures.
205+
fn make_test_bundle() -> TempFile {
206+
let projects_slice = &["wat-project".into()];
207+
let context = BundleContext {
208+
org: "wat-org",
209+
projects: Some(projects_slice.into()),
210+
release: None,
211+
dist: None,
212+
note: None,
213+
};
214+
215+
let source_files = ["bundle.min.js.map", "vendor.min.js.map"]
216+
.into_iter()
217+
.map(|name| SourceFile {
218+
url: format!("~/{name}"),
219+
path: format!("tests/integration/_fixtures/{name}").into(),
220+
contents: std::fs::read(format!("tests/integration/_fixtures/{name}"))
221+
.unwrap()
222+
.into(),
223+
ty: SourceFileType::SourceMap,
224+
headers: Default::default(),
225+
messages: Default::default(),
226+
already_uploaded: false,
227+
})
228+
.collect::<Vec<_>>();
229+
230+
build(context, &source_files, None).unwrap()
231+
}
232+
205233
#[test]
206234
fn test_url_to_bundle_path() {
207235
assert_eq!(url_to_bundle_path("~/bar").unwrap(), "_/_/bar");
@@ -229,37 +257,35 @@ mod tests {
229257

230258
#[test]
231259
fn build_deterministic() {
232-
let projects_slice = &["wat-project".into()];
233-
let context = BundleContext {
234-
org: "wat-org",
235-
projects: Some(projects_slice.into()),
236-
release: None,
237-
dist: None,
238-
note: None,
239-
};
240-
241-
let source_files = ["bundle.min.js.map", "vendor.min.js.map"]
242-
.into_iter()
243-
.map(|name| SourceFile {
244-
url: format!("~/{name}"),
245-
path: format!("tests/integration/_fixtures/{name}").into(),
246-
contents: std::fs::read(format!("tests/integration/_fixtures/{name}"))
247-
.unwrap()
248-
.into(),
249-
ty: SourceFileType::SourceMap,
250-
headers: Default::default(),
251-
messages: Default::default(),
252-
already_uploaded: false,
253-
})
254-
.collect::<Vec<_>>();
255-
256-
let file = build(context, &source_files, None).unwrap();
260+
let first = std::fs::read(make_test_bundle().path()).unwrap();
261+
let second = std::fs::read(make_test_bundle().path()).unwrap();
262+
assert_eq!(first, second, "bundle output should be deterministic");
263+
}
257264

258-
let buf = std::fs::read(file.path()).unwrap();
259-
let hash = Sha1::from(buf);
260-
assert_eq!(
261-
hash.digest().to_string(),
262-
"f0e25ae149b711c510148e022ebc883ad62c7c4c"
263-
);
265+
/// Canary against silent compression-method changes from upstream bundle
266+
/// writers. The allowlist is a starting bound, not a verified server
267+
/// contract. We can widen it if a real incompatibility surfaces.
268+
#[test]
269+
fn build_compression_method_canary() {
270+
let bundle = make_test_bundle();
271+
let mut archive =
272+
zip::ZipArchive::new(std::fs::File::open(bundle.path()).unwrap()).unwrap();
273+
274+
// Two source files plus the manifest.json that the source bundle
275+
// writer adds automatically.
276+
assert_eq!(archive.len(), 3, "unexpected bundle entry count");
277+
for i in 0..archive.len() {
278+
let entry = archive.by_index(i).unwrap();
279+
let method = entry.compression();
280+
assert!(
281+
matches!(
282+
method,
283+
zip::CompressionMethod::Stored | zip::CompressionMethod::Deflated
284+
),
285+
"entry {:?} uses {method:?}, outside the current allowlist: \
286+
verify backend compatibility before widening",
287+
entry.name(),
288+
);
289+
}
264290
}
265291
}
-4.4 KB
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.

tests/integration/build/upload.rs

Lines changed: 64 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
1-
use crate::integration::{AssertCommand, MockEndpointBuilder, TestManager};
2-
use regex::bytes::Regex;
31
use std::sync::atomic::{AtomicBool, Ordering};
4-
use std::sync::LazyLock;
5-
use std::{fs, str};
2+
3+
use crate::integration::test_utils::chunk_upload;
4+
use crate::integration::{AssertCommand, MockEndpointBuilder, TestManager};
65

76
#[cfg(all(target_os = "macos", target_arch = "aarch64"))]
87
#[test]
@@ -105,26 +104,13 @@ fn command_build_upload_apk_invlid_sha() {
105104
TestManager::new().register_trycmd_test("build/build-invalid-*-sha.trycmd");
106105
}
107106

108-
/// This regex is used to extract the boundary from the content-type header.
109-
/// We need to match the boundary, since it changes with each request.
110-
/// The regex matches the format as specified in
111-
/// https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html.
112-
static CONTENT_TYPE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
113-
Regex::new(
114-
r#"^multipart\/form-data; boundary=(?<boundary>[\w'\(\)+,\-\.\/:=? ]{0,69}[\w'\(\)+,\-\.\/:=?])$"#
115-
)
116-
.expect("Regex is valid")
117-
});
118-
119107
#[test]
120108
/// This test simulates a full chunk upload (with only one chunk).
121109
/// It verifies that the Sentry CLI makes the expected API calls to the chunk upload endpoint
122110
/// and that the data sent to the chunk upload endpoint is exactly as expected.
123111
/// It also verifies that the correct calls are made to the assemble endpoint.
124112
fn command_build_upload_apk_chunked() {
125113
let is_first_assemble_call = AtomicBool::new(true);
126-
let expected_chunk_body = fs::read("tests/integration/_expected_requests/build/apk_chunk.bin")
127-
.expect("expected chunk body file should be present");
128114

129115
TestManager::new()
130116
.mock_endpoint(
@@ -134,34 +120,35 @@ fn command_build_upload_apk_chunked() {
134120
.mock_endpoint(
135121
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/chunk-upload/")
136122
.with_response_fn(move |request| {
137-
let content_type_headers = request.header("content-type");
123+
let boundary = chunk_upload::boundary_from_request(request)
124+
.expect("content-type header should be a valid multipart/form-data header");
125+
126+
let body = request.body().expect("body should be readable");
127+
128+
let decompressed = chunk_upload::decompress_chunks(body, boundary)
129+
.expect("chunks should be valid gzip data");
130+
131+
assert_eq!(decompressed.len(), 1, "expected exactly one chunk");
132+
133+
// The CLI wraps the APK in a zip bundle with metadata.
134+
// Verify the bundle is a valid zip containing the APK.
135+
let chunk = decompressed.first().unwrap();
136+
let cursor = std::io::Cursor::new(chunk);
137+
let mut archive =
138+
zip::ZipArchive::new(cursor).expect("chunk should be a valid zip");
139+
let apk_entry = archive
140+
.by_name("apk.apk")
141+
.expect("bundle should contain the APK file");
142+
let expected_size =
143+
std::fs::metadata("tests/integration/_fixtures/build/apk.apk")
144+
.expect("fixture file should exist")
145+
.len();
138146
assert_eq!(
139-
content_type_headers.len(),
140-
1,
141-
"content-type header should be present exactly once, found {} times",
142-
content_type_headers.len()
147+
apk_entry.size(),
148+
expected_size,
149+
"APK size in bundle should match the fixture"
143150
);
144-
let content_type = content_type_headers[0].as_bytes();
145-
let boundary = CONTENT_TYPE_REGEX
146-
.captures(content_type)
147-
.expect("content-type should match regex")
148-
.name("boundary")
149-
.expect("boundary should be present")
150-
.as_bytes();
151-
let boundary_str = str::from_utf8(boundary).expect("boundary should be valid utf-8");
152-
let boundary_escaped = regex::escape(boundary_str);
153-
let body_regex = Regex::new(&format!(
154-
r#"^--{boundary_escaped}(?<chunk_body>(?s-u:.)*?)--{boundary_escaped}--\s*$"#
155-
))
156-
.expect("regex should be valid");
157-
let body = request.body().expect("body should be readable");
158-
let chunk_body = body_regex
159-
.captures(body)
160-
.expect("body should match regex")
161-
.name("chunk_body")
162-
.expect("chunk_body section should be present")
163-
.as_bytes();
164-
assert_eq!(chunk_body, expected_chunk_body);
151+
165152
vec![] // Client does not expect a response body
166153
}),
167154
)
@@ -212,13 +199,42 @@ fn command_build_upload_ipa_chunked() {
212199
.mock_endpoint(
213200
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/chunk-upload/")
214201
.with_response_fn(move |request| {
215-
let content_type_headers = request.header("content-type");
202+
let boundary = chunk_upload::boundary_from_request(request)
203+
.expect("content-type header should be a valid multipart/form-data header");
204+
205+
let body = request.body().expect("body should be readable");
206+
207+
let decompressed = chunk_upload::decompress_chunks(body, boundary)
208+
.expect("chunks should be valid gzip data");
209+
210+
assert_eq!(decompressed.len(), 1, "expected exactly one chunk");
211+
212+
// The CLI converts the IPA to an XCArchive structure and
213+
// zips it with a metadata file. Verify the chunk is a valid
214+
// zip containing the expected set of entries.
215+
let chunk = decompressed.first().unwrap();
216+
let mut archive = zip::ZipArchive::new(std::io::Cursor::new(chunk))
217+
.expect("chunk should be a valid zip");
218+
let entry_names: std::collections::HashSet<String> = (0..archive.len())
219+
.map(|i| archive.by_index(i).unwrap().name().to_owned())
220+
.collect();
221+
let expected_entries: std::collections::HashSet<String> = [
222+
"archive.xcarchive/Info.plist",
223+
"archive.xcarchive/Products/Applications/DemoApp.app/DemoApp",
224+
"archive.xcarchive/Products/Applications/DemoApp.app/Info.plist",
225+
"archive.xcarchive/Products/Applications/DemoApp.app/PkgInfo",
226+
"archive.xcarchive/Products/Applications/DemoApp.app/_CodeSignature/CodeResources",
227+
"archive.xcarchive/Products/Applications/DemoApp.app/embedded.mobileprovision",
228+
".sentry-cli-metadata.txt",
229+
]
230+
.into_iter()
231+
.map(String::from)
232+
.collect();
216233
assert_eq!(
217-
content_type_headers.len(),
218-
1,
219-
"content-type header should be present exactly once, found {} times",
220-
content_type_headers.len()
234+
entry_names, expected_entries,
235+
"IPA-derived xcarchive bundle should contain the expected entries",
221236
);
237+
222238
vec![] // Client does not expect a response body
223239
}),
224240
)

0 commit comments

Comments
 (0)