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

feat: module of utilities for integration tests #74

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JyJyJcr
Copy link
Contributor

@JyJyJcr JyJyJcr commented Apr 7, 2024

Proposed changes

This PR adds test_util module, utilities for integration tests of Nginx modules. The utilities were moved from tests/log_test.rs, and fixed to use the bundled Nginx used in bindings, the install path of which is recorded in nginx-sys::metadata::NGINX_INSTALL_DIR.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@dekobon
Copy link
Contributor

dekobon commented Apr 15, 2024

Hi there, once again thank you for all of your contributions. I talked over this PR with some of the other project maintainers and there is a strong opinion that this functionality may be better in another project/library that supplements ngx-rust. Would it be possible to do that rather than integrate it into the core SDK?

@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Apr 16, 2024

Of course possible (and I agree to split it from the core), but before changing let me clarify which is better, creating a new GitHub repository, or just separating it into a new subdirectory in this repository, just like nginx-sys?
Sidenote: we need to keep nginx-sys::metadata module to salvage build informations for integration tests in either case.

@bavshin-f5
Copy link
Collaborator

Sidenote: we need to keep nginx-sys::metadata module to salvage build informations for integration tests in either case.

There's nginx-sys::NGX_PREFIX that already contains the installation path (NULL-terminated u8 slice, so you need to strip the last byte and use std::os::unix::ffi::OsStrExt).

It's also optional and could be omitted by the buildsystem if you specify an empty --prefix (we do that for the official Windows builds), but that's only a problem with non-vendored Nginx source tree.

@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Apr 20, 2024

There's nginx-sys::NGX_PREFIX that already contains the installation path (NULL-terminated u8 slice, so you need to strip the last byte and use std::os::unix::ffi::OsStrExt).

Thank you for important information. Now test_util directly uses constants in bindings.rs (not only NGX_PREFIX but also other configurations, such as NGX_CONF_PATH).

@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Dec 22, 2024

Hi there, once again thank you for all of your contributions. I talked over this PR with some of the other project maintainers and there is a strong opinion that this functionality may be better in another project/library that supplements ngx-rust. Would it be possible to do that rather than integrate it into the core SDK?

Splitting the module to another project could cause incompatible (different version) two indirect dependencies to ngx crate. Such state would generate binding.rs twice, for two versions, resulting bad CI/CD performance.
Rather I add the feature test_util, and you can enable the module only in dev-dependencies.

@dekobon dekobon requested a review from bavshin-f5 December 23, 2024 17:47
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.

3 participants