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

test: use postgrest.read_stdout in io tests #3412

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

steve-chavez
Copy link
Member

It's easier to maintain this way in case there are new log lines.

Related to #3229 (comment)

It's easier to maintain this way in case there are new log lines.
@wolfgangwalther
Copy link
Member

This takes away from the previous test, because the log format itself is not tested anymore, right?

How about just doing snapshot tests for each log level, and thus saving the full log output for each? This way we'd have a much better picture of the log level differences.

@steve-chavez
Copy link
Member Author

This takes away from the previous test, because the log format itself is not tested anymore, right?

Just added back the previous regexes. I thought the user agent and roles on the log lines were irrelevant for this test but I'm cool with adding them back.

@steve-chavez
Copy link
Member Author

How about just doing snapshot tests for each log level, and thus saving the full log output for each? This way we'd have a much better picture of the log level differences.

I'd rather not invest more time on this for now. This was just a step for #3229, which I need for clearing an unknown pool issue #3214.

@steve-chavez
Copy link
Member Author

This is still an improvement over the current tests, I think. Will merge.

@steve-chavez steve-chavez merged commit 9d1dc78 into PostgREST:main Apr 15, 2024
13 checks passed
@steve-chavez
Copy link
Member Author

How about just doing snapshot tests for each log level, and thus saving the full log output for each? This way we'd have a much better picture of the log level differences.

Wanted to open an issue for this, but giving it more thought.. maybe snapshot testing is not adequate for logs because the lines order is not deterministic. The snapshots could change frequently and we'd have to update them everytime (more maintenance).

Currently we do sorted() on the log lines to make the output deterministic, which has been working fine.

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

Successfully merging this pull request may close these issues.

2 participants