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 test ❌ #39

Merged
merged 6 commits into from
Nov 4, 2022
Merged

Test test ❌ #39

merged 6 commits into from
Nov 4, 2022

Conversation

DanGould
Copy link
Contributor

#13 passed CI when it should not have, because the loin server failed to connect. However, it passes with a successful payjoin on my machine. I'm pushing this to run CI and see if I can replicate the problem.
https://github.com/chaincase-app/nolooking/actions/runs/3325555814
/jobs/5498363364

@DanGould
Copy link
Contributor Author

Aha, the test passes despite a panicked thread. Two things need to be done:

  1. make sure if critical threads panic the test fails. assert!(has_opened_channel)
  2. loop until server@3000 is accessible from 3010. I pj panics because of a race. with the 2 second timeout

@DanGould DanGould changed the title Test test Test test ❌ Oct 30, 2022
@DanGould
Copy link
Contributor Author

oh.. I accidentally killed the nginx events {} section which invalidated the configuration.

I missed it because the test was passing regardless of a channel open

@DanGould DanGould force-pushed the test-test branch 3 times, most recently from 4a888d3 to f5083d1 Compare November 2, 2022 18:20
@@ -0,0 +1 @@
# this file keeps the directory around for tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be an empty .gitignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I force-pushed a .gitignore. The rest is the same

@nickfarrow
Copy link
Collaborator

yeah you knew it, damn docker L179 integration.rs needs 172.17.0.1.

Also maybe cherry-pick /pull this commit onto your test-test branch, makes tests fail depending on test success.
nickfarrow@f97d3ce
If the channel opens, we set the Fixture to have test_succeedeed==true, which we later use in drop() to maybe panic.

@DanGould
Copy link
Contributor Author

DanGould commented Nov 3, 2022

@nickfarrow Huzzah!!! you fixed it! This one was a headache to find.

Binding to 172.17.0.2 doesn't run on my mac however, so we need a dynamic var to bind to.

DanGould and others added 5 commits November 3, 2022 19:17
Without binding to the bridge the gateway fails on GitHub actions.
By adding a `test_succeeded` bool to the test `Fixture` we can
update it to `true` once the test case has been completed.
After `drop(self)` runs the docker cleanup, if `test_succeeded` is
false then we force a panic to reflect the failed test.
@DanGould DanGould requested a review from nickfarrow November 3, 2022 23:25
@nickfarrow
Copy link
Collaborator

nickfarrow commented Nov 3, 2022

Nice! LGTM. Let me run locally

@DanGould DanGould merged commit ff1e919 into payjoin:master Nov 4, 2022
@DanGould DanGould deleted the test-test branch November 4, 2022 14:31
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.

2 participants