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

Issue/434 missing buffer logs #435

Closed
wants to merge 3 commits into from
Closed

Conversation

YOU54F
Copy link
Member

@YOU54F YOU54F commented Jun 8, 2024

fixes #434

I believe this issue stems from one of the pact crates having a different version of the tracing crate used for logging.

This seems like an awkward position to be in, when upgrading dependencies of each of the indiv pact rust crates, especially as two sit across different repos (pact_mock_server / pact-plugin-driver).

Would it make sense to crate a pact_logging crate that wraps the required deps for logging, and this singular crate can be imported by all the other pact rust crates.

If we need to update the logging deps, we update them in the pact_logging crate, and then can use the workspace override to patch all crates to use the same versions (via pact_logging)

add smoke test, needs to be executed in isolation, as it uses the global logging provider and thusly is affected by other tests in the same process
@YOU54F YOU54F changed the title Issue/134 missing buffer logs Issue/434 missing buffer logs Jun 8, 2024
@YOU54F YOU54F marked this pull request as ready for review June 8, 2024 22:49
@@ -12,3 +12,7 @@ resolver = "2"

[patch.crates-io]
onig = { git = "https://github.com/rust-onig/rust-onig", default-features = false }
pact_models = { path = "./pact_models" }
pact_matching = { path = "./pact_matching" }
pact_mock_server = { git = 'https://github.com/pact-foundation/pact-core-mock-server.git', branch = "issue/134_missing_buffer_logs" }
Copy link
Member Author

Choose a reason for hiding this comment

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

note these are reliant on patches in branches which pull in the updated pact-models code from this pr.

will need co-ordinating with release of pact models, then pact-plugin-driver, which is required by pact-core-mock-server, which is required by crates in this repo.

println!("{}",logs);
assert_ne!(logs,"", "logs are empty");

pactffi_cleanup_mock_server(port);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the assertion above it fails, this line will not be executed.

@rholshausen
Copy link
Contributor

I'm wondering if we use the patch block to force the logging crate version, this will fix this issue. It does mean we need some way of working out when to update it. But the logging crates have been quite stable.

@rholshausen
Copy link
Contributor

I'm going to close this PR, as it can't be merged in the current state. I'll then cherry pick the changes from your branch.

@rholshausen
Copy link
Contributor

This was not caused by the logging crates, but by the FFI InMemory sink which uses the global static LOG_BUFFER in the pact_matching crate. This is required to be the same version across all crates (i.e. mock server crate).

@YOU54F
Copy link
Member Author

YOU54F commented Jun 11, 2024

ahhh great, good detective work @rholshausen

@YOU54F YOU54F deleted the issue/134_missing_buffer_logs branch July 31, 2024 03:58
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.

0.4.20 pactffi_mock_server_logs empty - mismatch in tracing library
2 participants