Reducing test output verbosity about polish HOT 23 CLOSED

@U007D
Noted. I will have both requirements satisfied within a matter of hours.

Thanks!

from polish.

Hi, @Alkass, I would really appreciate that, thank you!

Also, I took a look at the source code, and saw that changing the set_attribute()/set_attributes() function signatures to support fluent use:

pub fn set_attribute(&mut self, attribute: i64) -> &mut Self;
pub fn set_attributes(&mut self, attributes: i64) -> &mut Self;

would enable a Builder Pattern style use of TestRunner like so:

    TestRunner::new()
               .set_attributes(attributes.minimize_output | attributes.disable_final_stats)
               .run_tests(...

which would be very elegant to use, and would not break existing code. What do you think?

from polish.

@U007D
As promised, both requirements have been satisfied. polish = "0.9.0" will have all the changes. Please go ahead and make sure everything works to your satisfaction so I can close this issue.

Thanks,
Fadi

from polish.

This output is So. Much. Cleaner. Fantastic, @Alkass, thank you!

from polish.

Any chance you would be willing to accommodate one more favor?

✅ hesl::unorm::unit_tests::unorm::UNorm::new(): calling with no input succeeds (4ns)

Now that they're gone, I found that the timing information was informative; I got a sense of which operations in Rust were expensive, almost by osmosis. You've already gone well beyond, but if it's simple to plop the times in, it would be icing on the cake. If not, truly, no problem--the main issues have been resolved.

Wonderful work. Thank you again, @Alkass.

from polish.

Wow. You are a force of nature! :)
I'll get a chance to run it later this evening (also PT). Thank you, once again!

from polish.

I ran 0.9.4 and the output looks beautiful.

BTW, I took a look at the diff for this checkin and noticed a small issue. Looking at the source code for polish, it looks like the times being reported are actually µs, not ns.

From test_case.rs

        let starting_time: i32 = time::now().tm_nsec;
        let mut status: TestCaseStatus = (test.exec)(&mut logger);
        let ending_time: i32 = time::now().tm_nsec;
...
            duration: (ending_time - starting_time) / 1000,
...
            println!("{} {}::{}: {} ({}ns)", mark, self.module_path, test.title, formatted_criteria, test_info.duration);

It's a small thing, but I thought you might want to know.

Thanks so much for the speedy fixes, my friend! If you are ever in the Seattle area, let me know and I'll buy you a cup of your favorite beverage!

All the best,
Brad (aka U007D)

from polish.

@U007D
My pleasure, sir! I'm actually happy someone took the time to report issues and make suggestions. According to the Rust Package Manager site (Crates.io), a little over 1300 people have downloaded the framework, yet this is the first time I hear from someone who's actually using it in a project. I'm probably more happy with this issue than you are with the fixes you received.

Good catch on the time issue. I believe i'd initially used implemented it to work with milliseconds and was reading seconds from time::now() then changed my mind to nanoseconds and forgot to remove the division by 1000. I will certainly have this fixed ASAP.

As per your get-together suggestion, I do travel across the country quite a lot throughout the year. As a matter of fact, this past week alone I have been on 6 airplanes traveling back and forth between Austin, San Diego, and LA. I will sure be somewhere around Seattle area in the next couple of months. A cup of coffee from Starbucks will do ^_^

Thanks again, Brad!

Fadi

from polish.

I might introduce a few attributes that allow a change to this behavior next

from polish.

NOICE!!! Thank you, Santa!! :)

That looks fantastic--it's a real pleasure to use a library where the author cares about making a beautiful API. Much appreciated!

Merry Christmas and happy holidays to you, too!

from polish.

Hi,
The behaviour you're suggesting can be defined with a new attribute. You'll end up with something like:

let mut t = TestRunner::new();
t.set_attribute(attributes.minimize_output);
t.set_attribute(attributes.disable_final_stats);
t.run_test(TestCase::new("title", "criteria", Box::new(|_: &mut Logger| -> TestCaseStatus {TestCaseStatus::UNKNOWN})));

Please note that the minimize_output attribute is not implemented already, but I'm willing to implement it if you find this an appropriate solution to your problem.

from polish.

@Alkass this looks really good. This is the output I'm seeing:

❌ calling with no input succeeds
✅ calling with a valid-for-a-unorm f64 value succeeds
✅ calling with the minimum unorm f64 value succeeds
✅ calling with the maximum unorm f64 value succeeds
✅ calling with a too-small-for-a-unorm f64 value fails
✅ calling with a too-large-for-a-unorm f64 value fails

The only thing that I would ask is for the messages to be namespaced with the path to the TestRunner, so it's possible to easily understand which test is running. For example the first message above ideally would read (in my particular case):

❌ ada::hesl::unorm::tests::UNorm::new(): calling with no input succeeds
...

where the corresponding test is defined via that path:

#[test]
fn tests() {
    TestRunner::new()
               .set_attributes(TEST_RUNNER_ATTRIBUTES.disable_final_stats | TEST_RUNNER_ATTRIBUTES.minimize_output)
               .run_tests(vec![
        TestCase::new("UNorm::new()", "calling with no input succeeds", Box::new(|_logger: &mut Logger| -> TestCaseStatus {
...

If that is too time-consuming or tricky, I understand. But it would make it possible to read the abbreviated tests at a glance and jump to the problem.

Huge thanks for jumping on this, @Alkass!

from polish.

So, it seems like the only thing we could make use of is the module_path! macro. Problem is, if I call this from a runner function, I get "polish::test_case". This function would need to be handed over as an argument to a helper function.

What I can do is provide an optional set_module_path(&mut self, module_path: &'static str) -> &mut Self function that you can call as follows:

TestRunner::new()
.set_module_path(module_path!())
.set_attributes(...)
.run_tests(...)

Is this something you think you could live with?

from polish.

Yes, I think that is a good solution given the reality of how module_path! works. And for those that don't want or need the extra specificity, .set_module_path() is optional. I like it--nice!

from polish.

@U007D,
Go ahead and check it out now.

Fadi

from polish.

Right on. Will have the time added by the end of the day today (PT).

Thanks @U007D

from polish.

@U007D
Done. Changes are in 0.9.4 onward

from polish.

Thanks @U007D
Please keep me posted

Fadi

from polish.

Sounds great, @Alkass. I'll reach out to you with my contact info. I look forward to it!

Re: units, yes, I understand--I've done that before too. :) I'm not sure which way you're planning to go in terms of units, but I can tell you that I do appreciate the reporting of a (typically) small number of microseconds as opposed to the visually noisier large numbers of nanoseconds when glancing over my test results.

from polish.

Are there any more suggestions you'd like to add to this thread or should I go ahead and close the issue?

Fadi

from polish.

Unless you know of some way to reduce Cargo's test harness verbosity, then I think we're all good here--it's fine to close this issue--thank you.

(While polish's documentation shows test code running from fn main(), all my tests run under the #[test] attribute, and thus Cargo adds its own commentary to polish's output; but my understanding is Cargo's behavior in this regard is not configurable/overridable).

BTW, I've sent you an e-mail to your git commit e-mail address.

from polish.

@U007D
Check out the new changes and take a look at time_unit.rs

Merry Christmas! ^_^

from polish.

The love is real in this issue.

from polish.