Skip to content

Specifi nginx via environment variable or skip test - #1237

Merged
spacebear21 merged 1 commit into
payjoin:masterfrom
zealsham:nginx-env
Dec 29, 2025
Merged

spacebear21 merged 1 commit into
payjoin:masterfrom
zealsham:nginx-env

Conversation

@zealsham

Copy link
Copy Markdown
Collaborator

This pr addresses #1227, it specifies an enironment variable for nginx which will be used during test run. it also removes nginxWithStream package from the dev shell.

i used Claude AI to help me with nix

Pull Request Checklist

Please confirm the following before requesting review:

@zealsham
zealsham marked this pull request as draft December 17, 2025 22:24
@coveralls

coveralls commented Dec 17, 2025

Copy link
Copy Markdown
Collaborator

Pull Request Test Coverage Report for Build 20374581801

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 58 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.5%) to 82.905%

Files with Coverage Reduction New Missed Lines %
ohttp-relay/src/lib.rs 1 79.61%
ohttp-relay/src/bootstrap/ws.rs 57 0.0%
Totals Coverage Status
Change from base Build 20347892595: -0.5%
Covered Lines: 9670
Relevant Lines: 11664

💛 - Coveralls

@zealsham
zealsham marked this pull request as ready for review December 18, 2025 08:26

@nothingmuch nothingmuch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good, I have only one suggestion to use an error enum { NginxNotAvailable, UnknownError(Box<dyn Error>) } instead of also making the NginxNotAvailable condition into a trait object, since these two branches are handled quite differently

Comment thread ohttp-relay/tests/integration.rs Outdated
This pr addresses payjoin#1227, it specifies an enironment variable
for nginx which will be used during test run. it also
removes nginxWithStream package from the dev shell.

introduce error enum for nginx failures

introduce error enum for nginx failures

@nothingmuch nothingmuch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK.

since i opened the issue and suggested this solution i think another maintainer should at least concept ACK before merging

@spacebear21 spacebear21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

partial tACK on c8017eb - I tested contrib/test.sh locally without nix and confirmed it skips the problematic tests.

We should be able to also remove the "add nginxWithStream to PATH" step in CI?

@zealsham

Copy link
Copy Markdown
Collaborator Author

partial tACK on c8017eb - I tested contrib/test.sh locally without nix and confirmed it skips the problematic tests.

We should be able to also remove the "add nginxWithStream to PATH" step in CI?

should that be done in this PR or a different one ?

@spacebear21

Copy link
Copy Markdown
Collaborator

I'm actually not sure if it can be removed, looks like we still need nginxWithStream when setting the new environment variable.

@spacebear21
spacebear21 merged commit 5902303 into payjoin:master Dec 29, 2025
9 checks passed
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.

4 participants