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

Strange indentation in libtest output #104092

Closed
RalfJung opened this issue Nov 7, 2022 · 6 comments · Fixed by #118548
Closed

Strange indentation in libtest output #104092

RalfJung opened this issue Nov 7, 2022 · 6 comments · Fixed by #118548
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 7, 2022

libtest output recently started indenting some but not all test results:

$./x.py test library/alloc --stage 0 --test-args "--format pretty" --no-doc
running 373 tests
test raw_vec::tests::allocator_param ... ok
test raw_vec::tests::reserve_does_not_overallocate ... ok
test raw_vec::tests::zst ... ok
test alloc::tests::allocate_zeroed ... ignored
test raw_vec::tests::zst_reserve_panic - should panic ... ok
test raw_vec::tests::zst_reserve_exact_panic - should panic ... ok
test alloc::tests::alloc_owned_small                         ... ok   <!----- notice this one!
test collections::binary_heap::tests::test_iter_rev_cloned_collect ... ok
[...]
test collections::linked_list::tests::test_drop ... ok
test collections::linked_list::tests::test_drop_panic ... ok
test collections::vec_deque::tests::bench_push_front_100     ... ok  <!---- and these!
test collections::vec_deque::tests::bench_push_back_100      ... ok
test collections::vec_deque::tests::bench_pop_back_100       ... ok
test collections::vec_deque::tests::bench_pop_front_100      ... ok
test collections::vec_deque::tests::test_swap_front_back_remove ... ok
test collections::vec_deque::tests::test_insert ... ok
test collections::vec_deque::tests::test_get ... ok
test collections::vec_deque::tests::test_get_mut ... ok
test collections::vec_deque::tests::bench_retain_whole_10000 ... ok

Seems like benchmarks are indented but regular tests are not.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2022

Cc @thomcc any idea which change might have caused this? I think this must be fairly recent but haven't tried bisecting yet.

@jyn514 jyn514 added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. labels Apr 9, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 9, 2023
@jyn514 jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 9, 2023
@m-ou-se m-ou-se added P-low Low priority E-help-wanted Call for participation: Help is requested to fix this issue. labels Apr 12, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Apr 12, 2023

Is this still reproducable?

@RalfJung
Copy link
Member Author

With #108659 this can no longer be tested using x.py, since bootstrap now does test output formatting itself.

So this now needs a little cargo project with a test and a benchmark, to check what libtest actually does.

@ehuss
Copy link
Contributor

ehuss commented Apr 12, 2023

It is because benches are dynamic, and they always use padding (and tests are static):

https://github.com/ehuss/rust/blob/6b57a344cba18b18178cdcf609d68880f33cc6eb/library/test/src/types.rs#L90-L97

I'm not sure why padding is different for static vs dynamic.

Here is a repro:

#![feature(test)]
extern crate test;

#[test]
fn foo() {}

#[bench]
fn short_name(b: &mut test::Bencher) {
    b.iter(|| 1);
}

#[bench]
fn this_is_a_really_long_name(b: &mut test::Bencher) {
    b.iter(|| 1);
}

@cuviper
Copy link
Member

cuviper commented Apr 12, 2023

Seems like that could use context if the bench is only being run as a test, then don't pad.

The padding is desirable for benchmark output so the numeric values line up, for easier visual comparison. You could argue either way about tests -- maybe ok/FAILED would stand out even more with padded alignment.

@m-ou-se m-ou-se removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 19, 2023
@Enselic
Copy link
Member

Enselic commented Dec 2, 2023

PR with regression tests and a fix (in that order): #118548

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 6, 2023
libtest: Fix padding of benchmarks run as tests

### Summary

The first commit adds regression tests for libtest padding.

The second commit fixes padding for benches run as tests and updates the blessed output of the regression tests to make it clear what effect the fix has on padding.

Closes rust-lang#104092 which is **E-help-wanted** and **regression-from-stable-to-stable**

### More details

Before this fix we applied padding _before_ manually doing what `convert_benchmarks_to_tests()` does which affects padding calculations. Instead use `convert_benchmarks_to_tests()` first if applicable and then apply padding afterwards so it becomes correct.

Benches should only be padded when run as benches to make it easy to compare the benchmark numbers. Not when run as tests.

r? `@ghost` until CI passes.
@bors bors closed this as completed in 5431404 Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
Development

Successfully merging a pull request may close this issue.

7 participants