Skip to content

Conversation

@cerdelen
Copy link
Contributor

@cerdelen cerdelen commented Feb 8, 2026

New PR to fix issues after revert of #10798

Fix: #10698

@sylvestre
Copy link
Contributor

do we need that many tests ? thanks

@cerdelen
Copy link
Contributor Author

cerdelen commented Feb 8, 2026

Every test is a different edge case around the chunking process. I thought rather have a few more than a few too little.

@github-actions
Copy link

github-actions bot commented Feb 8, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/tail/tail-n0f is now passing!

@sylvestre
Copy link
Contributor

yeah my point was that there is some duplications in the tests and they might be merged with a loop with different args / expectations

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 8, 2026

Merging this PR will improve performance by 58.43%

⚡ 2 improved benchmarks
✅ 282 untouched benchmarks
⏩ 38 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation unexpand_many_lines[100000] 300.6 ms 189.7 ms +58.4%
Simulation unexpand_large_file[10] 630.1 ms 397.7 ms +58.43%

Comparing cerdelen:unexpand_buffer (cdff1df) with main (7b0e7e4)

Open in CodSpeed

Footnotes

  1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

#[allow(clippy::cognitive_complexity)]
fn unexpand_line(
buf: &mut Vec<u8>,
#[allow(clippy::too_many_arguments)]
Copy link
Contributor

Choose a reason for hiding this comment

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

for a different PR but it should be refactored
way too many arguments now

Copy link
Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

still waiting for some of the tests content to be deduplicated

@cerdelen
Copy link
Contributor Author

cerdelen commented Feb 9, 2026

Would be 1 function like this be acceptable?

#[test]
fn test_buffered_read_edgecase_behaviour() {
    let test_cases = [
        {
            // input has newlines in first chunk and has leading spaces after newline in chunk
            let mut input = vec![b'0'; 180];
            input.push(b'\n');
            input.extend([b' '; 8]);
            input.extend([b'0'; 3897]);
            input.push(b'\n');

            let mut expected = vec![b'0'; 180];
            expected.push(b'\n');
            expected.push(b'\t');
            expected.extend([b'0'; 3897]);
            expected.push(b'\n');
            (input, expected)
        },
        ... // other edgecases
    ]

    new_ucmd!()
        .pipe_in(input)
        .succeeds()
        .stdout_only(String::from_utf8(expected).unwrap());
}

@sylvestre
Copy link
Contributor

sure

@oech3

This comment was marked as resolved.

@github-actions
Copy link

github-actions bot commented Feb 9, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/tail/tail-n0f is now passing!

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add keywords cspell does not like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i did, i am confused why cspell marks them as error. I double checked and behaviour is written correctly. Weird!

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, cspell accepts en_US only? behavior

@github-actions
Copy link

github-actions bot commented Feb 9, 2026

GNU testsuite comparison:

Congrats! The gnu test tests/tail/tail-n0f is now passing!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/pr/bounded-memory is no longer failing!

@sylvestre sylvestre merged commit 04b6f0a into uutils:main Feb 10, 2026
155 checks passed
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.

unexpand: unexpand /dev/zero panic

3 participants