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

Increases verbosity of pgx-tests in various places #665

Merged
merged 11 commits into from
Sep 9, 2022

Conversation

BradyBonnette
Copy link
Contributor

@BradyBonnette BradyBonnette commented Aug 31, 2022

It was brought up in issue #324 that any errors in pgx-tests that resulted from a failed CREATE EXTENSION call were not propagated properly back to the user. While some of it could be displayed by running the test with the --nocapture flag, it's less than ideal since normal Rust tests would at least output something more meaningful instead of simply saying "failed" as it does in this case.

Upon inspection it turns out that there were other places where more verbose output should be warranted. These changes aims to make other errors more visible as well and include other small tweaks along the way.

Also note that these changes can also be referenced as part of #612

@BradyBonnette
Copy link
Contributor Author

Here is an example of output before any changes applied. This is a result of an invalid Postgres query by running cargo test -p aggregate --lib:

image

Here is the output with the changes implemented, throwing the same error:

image

It should be noted that I used the same exact command -- I did not append --nocapture at the end.

Because of the way the test framework is currently set up, there may be some instances where a single test failure means you get a huge infodump of other abruptly halted tests. Typically this is because the tests are behind a shared mutex that will shut down any running postgres instances and fail them outright -- all due to one failure. In those situations, the other abruptly-halted tests will display something along the lines of:

image

Though, in the above screenshot, the reason for the failure is because I specifically injected a failure in the CREATE EXTENSION query, which would technically fail on every single test. It feels sorta "spam-y" but with the way the framework is built, there was no other option.

Here is another example of a framework setup error that happens when a postgres client fails to set up properly:

image

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

This looks wonderful! I think the only major Thing is clawing back a bit of the vertical height and making sure we aren't distracting from the important parts. I agree that a little bit of "spam" isn't so bad but we should generally prefer to fight against giving up height in the terminal as excess noise will increase the tendency to tune out the test output.

Likewise, we should generally make anything that the user will see again and again look more subdued, while gearing in the highlighting on the things that will change every time.

If something needs to be more verbose, we can offer toggles for that. We could also piggyback off the setting for RUST_BACKTRACE instead of asking for a unique configuration just for PGX, cranking everything to "maximum volume" when they use "full", and up slightly when they use 1, IF we think that's appropriate (probably "yes" if it's implicitly "a backtrace", more dubious if it's not definitely a trace).

pgx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgx-pg-config/src/lib.rs Outdated Show resolved Hide resolved
pgx-pg-config/src/lib.rs Outdated Show resolved Hide resolved
pgx-tests/src/framework.rs Outdated Show resolved Hide resolved
pgx-tests/src/framework.rs Outdated Show resolved Hide resolved
cargo-pgx/src/command/install.rs Show resolved Hide resolved
BradyBonnette and others added 3 commits September 1, 2022 10:16
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
@BradyBonnette
Copy link
Contributor Author

@workingjubilee

Here are some updated screenshots of errors that have been abbreviated as per your suggestions.

Without RUST_BACKTRACE=1:

image

Same error with RUST_BACKTRACE=1 (remaining stuff cut off from the screenshot because this 4k monitor doesnt have that much real estate):

image

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

I like this! I did mention a slight formatting adjustment for sliiightly prettier and more searchable errors on Discord. By and large though, this looks nice!

@BradyBonnette
Copy link
Contributor Author

Yup, working on those now and getting feedback from you :)

@BradyBonnette
Copy link
Contributor Author

@workingjubilee Quick update after we discussed some things.

Example of a simple error without RUST_BACKTRACE=1:

image

Example with a backtrace:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants