test(mono): Refactor CL merge integration test #1915
Conversation
Signed-off-by: miliu2cc <miliu2cc@gmail.com>
Signed-off-by: miliu2cc <miliu2cc@gmail.com>
There was a problem hiding this comment.
Pull request overview
Refactors the mono CL merge integration test into a phased workflow with new helper functions for CL/API operations and additional verification steps around conflicts, rebases, and merge results.
Changes:
- Splits the integration scenario into multiple “phases” (create CLs, permission check, merge/rebase conflict handling, final verification).
- Introduces helper functions for CL API calls (
merge,update-branch,detail,status, etc.) and blob/status verification. - Adds DB- and API-based verification steps (including direct DB updates) to drive/validate conflict-resolution flows.
Comments suppressed due to low confidence (1)
mono/tests/cl_merge_integration.rs:851
create_clconfiguresgit config user.emailfrom theemailparameter, but the call sites passTEST_USER_A/TEST_USER_B(usernames) rather than actual emails. If server-side logic relies on commit author email, this can break attribution/ownership checks and make the test less representative. Pass a real email value (reintroduce the constants or useuser_a@test.com/user_b@test.com).
// Create CL-1 (user_a)
let cl1 = create_cl(
vm,
"repo_a",
TEST_USER_A,
TEST_TOKEN_A,
TEST_USER_A,
vec![
("common.txt", "Initial content by user_a"),
("repo_a/file1.txt", "Content A1"),
],
)
.await
.context("Failed to create CL-1")?;
// Create CL-2 (user_b)
let cl2 = create_cl(
vm,
"repo_b",
TEST_USER_B,
TEST_TOKEN_B,
TEST_USER_B,
vec![
("common.txt", "Modified by user_b - conflicts with CL-1!"),
("repo_b/file2.txt", "Content B1"),
],
| /// Verify blob content and print actual vs expected | ||
| async fn verify_blob( | ||
| vm: &mut qlean::Machine, | ||
| path: &str, | ||
| refs: &str, | ||
| expected: &str, | ||
| ) -> Result<()> { | ||
| let resp = exec_check( | ||
| vm, | ||
| &format!( | ||
| "curl -s -X POST http://127.0.0.1:8000/api/v1/cl/{}/update-branch", | ||
| cl_link | ||
| "curl -s 'http://127.0.0.1:8000/api/v1/blob?path={}&refs={}'", | ||
| path, refs | ||
| ), | ||
| ) | ||
| .await?; | ||
| Ok(update_cmd) | ||
| let actual = resp.trim().trim_matches('"').to_string(); | ||
| if actual != expected { | ||
| tracing::warn!( | ||
| " Blob mismatch: {}:{} expected '{}', got '{}'", | ||
| path, | ||
| refs, | ||
| expected, | ||
| actual | ||
| ); | ||
| anyhow::bail!( | ||
| "Blob mismatch: {}:{} expected '{}', got '{}'", | ||
| path, | ||
| refs, | ||
| expected, | ||
| actual | ||
| ); | ||
| } | ||
| tracing::info!(" Verified {}@{} = '{}'", path, refs, expected); | ||
| Ok(()) |
There was a problem hiding this comment.
verify_blob assumes /api/v1/blob returns a raw quoted string and compares resp.trim().trim_matches('"') to expected, but the API returns CommonResult<String> JSON (with data + req_result). This will cause false failures (or accidental passes via substring matching elsewhere). Parse the JSON and compare the data field (and also assert req_result == true) instead of trimming quotes.
| tracing::info!(" Updating CL-2 base hash to latest main via database"); | ||
| // Directly update CL-2 hashes in database | ||
| exec_check(vm, &format!( | ||
| r#"docker exec {} psql -U {} -d {} -c "UPDATE mega_cl SET from_hash = '{}', to_hash = '{}' WHERE link = '{}'""#, |
There was a problem hiding this comment.
The SQL command string in Phase 9 ends with ... WHERE link = '{}'"", which produces an extra/unmatched double-quote in the shell command and is likely to fail with a syntax error. Fix the quoting so the psql -c argument is wrapped in a single balanced quote pair (or avoid nested shell quoting by using single quotes around the SQL or a heredoc).
| r#"docker exec {} psql -U {} -d {} -c "UPDATE mega_cl SET from_hash = '{}', to_hash = '{}' WHERE link = '{}'""#, | |
| r#"docker exec {} psql -U {} -d {} -c "UPDATE mega_cl SET from_hash = '{}', to_hash = '{}' WHERE link = '{}'"#, |
| // Get CL link | ||
| let resp = exec_check(vm, r#"curl -s -X POST http://127.0.0.1:8000/api/v1/cl/list -H "Content-Type: application/json" -d '{"pagination":{"page":1,"per_page":10},"additional":{"status":"open","sort_by":"created_at","asc":false}}'"#).await?; | ||
| let json: Value = serde_json::from_str(&resp)?; | ||
| let cl_link = json["data"]["items"][0]["link"] | ||
| .as_str() | ||
| .ok_or_else(|| anyhow::anyhow!("CL link not found in response"))? | ||
| .unwrap_or("") | ||
| .to_string(); | ||
|
|
||
| let cl_author = items[0]["author"].as_str().unwrap_or("unknown"); | ||
|
|
||
| tracing::info!(" Created CL: {} (author: {})", cl_link, cl_author); | ||
|
|
||
| tracing::info!(" Created CL: {}", cl_link); | ||
| Ok(cl_link) |
There was a problem hiding this comment.
create_cl derives the CL link by indexing into json["data"]["items"][0] without checking req_result, that data/items exist, or that the list is non-empty. If the API returns an error or no open CLs, this returns an empty string and the rest of the test will fail in confusing ways. Parse the response as a CommonResult<CommonPage<_>>-shaped value (or at least check req_result == true, ensure items is a non-empty array, and bail with err_message if not).
| async fn update_cl_status( | ||
| vm: &mut qlean::Machine, | ||
| cl: &str, | ||
| status: &str, | ||
| token: &str, | ||
| ) -> Result<()> { | ||
| exec_check(vm, &format!("curl -s -X POST -H 'Authorization: Bearer {}' http://127.0.0.1:8000/api/v1/cl/{}/status -H 'Content-Type: application/json' -d '{{\"status\":\"{}\"}}'", token, cl, status)).await?; | ||
| Ok(()) |
There was a problem hiding this comment.
update_cl_status only checks curl’s process exit code; curl will typically exit 0 even on HTTP 4xx/5xx unless --fail/--fail-with-body is used, and the response body isn’t parsed to confirm req_result. This can make the test proceed as if the status update succeeded when it didn’t. Capture and parse the JSON response and fail the test when req_result is false (and consider adding --fail-with-body to make HTTP errors non-silent).
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a723dfa9cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| .await?; | ||
| Ok(update_cmd) | ||
| let actual = resp.trim().trim_matches('"').to_string(); |
There was a problem hiding this comment.
verify_blob treats /api/v1/blob as if it returns a raw string (resp.trim().trim_matches('"')), but this API returns a CommonResult<String> JSON envelope (see get_blob_string in mono/src/api/router/preview_router.rs). In Phase 3, that makes actual become the full JSON payload instead of the blob text, so comparisons against literals like Initial content by user_a fail even when the merge is correct, causing false test failures whenever the Phase 3 path is executed.
Useful? React with 👍 / 👎.
| let cl_link = json["data"]["items"][0]["link"] | ||
| .as_str() | ||
| .ok_or_else(|| anyhow::anyhow!("CL link not found in response"))? | ||
| .unwrap_or("") |
There was a problem hiding this comment.
The refactor now reads json["data"]["items"][0]["link"].as_str().unwrap_or("") without checking req_result or whether items is empty, so a transient /cl/list delay/error silently yields an empty link. That propagates into later calls like /api/v1/cl//status or merge/update operations, producing misleading downstream failures instead of failing early at CL creation with a clear error.
Useful? React with 👍 / 👎.
14ddd0d
3 participants
No description provided.