ci: 🚭 fix summonerd smoke tests #4358
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cratelyn
added
A-tooling
cratelyn
changed the title
env in summoner smoke test shebang
cratelyn
force-pushed
the
kate/local-summoner-test-running-tweaks
branch
from
36ba425 to
a022083
Compare
cratelyn
force-pushed
the
kate/local-summoner-test-running-tweaks
branch
from
7b745c9 to
a4edeca
Compare
cratelyn
force-pushed
the
kate/local-summoner-test-running-tweaks
branch
from
a4edeca to
7b86267
Compare
cratelyn
force-pushed
the
kate/local-summoner-test-running-tweaks
branch
from
7b86267 to
269ecc7
Compare
cratelyn
commented
May 9, 2024
cratelyn
commented
May 9, 2024
cratelyn
commented
May 9, 2024
cratelyn
commented
May 9, 2024
This was referenced May 9, 2024
cratelyn
commented
May 9, 2024
| @@ -58,6 +58,7 @@ async fn handle_bid(app: &mut App, to: Address, from: AddressIndex, bid: &str) - | |||
| planner.output(value, to); | |||
| let plan = planner | |||
| .memo("E PLURIBUS UNUM".into()) | |||
| .memo_return_address(app.config.full_viewing_key.payment_address(from).0) | |||
There was a problem hiding this comment.
this is the fix 🩹
everything else in this branch is tweaks to facilitate debugging, and confirming this does in fact fix the issue.
hdevalence
approved these changes
May 9, 2024
conorsch
approved these changes
May 9, 2024
| @@ -41,7 +67,7 @@ cargo run --quiet --release --bin pd -- start --home "${HOME}/.penumbra/testnet_ | |||
| pd_pid="$!" | |||
|
|
|||
| # Ensure processes are cleaned up after script exits, regardless of status. | |||
| trap 'kill -9 "$cometbft_pid" "$pd_pid"' EXIT | |||
| trap 'kill -9 "$cometbft_pid" "$pd_pid"' EXIT INT | |||
There was a problem hiding this comment.
Thank you for the super detailed commit messages. 🙏
conorsch
reviewed
May 9, 2024
this is a small noöp refactor. we don't need to acquire a handle to the current async runtime, if we are attempting to spawn onto the *current* runtime. by default, `tokio::spawn()` does that for us.
this adds `cometbft` (defined above) to the `devShell`. this argument is threaded down into `pkgs.mkShell`, see: - https://nixos.org/manual/nixpkgs/stable/#sec-pkgs-mkShell otherwise, `which cometbft` won't find cometbft. this was needed for me to run the summoner smoke test script locally.
- 🚭 use `env` in summoner smoke test shebang - 🚀 add `/tmp/summonerd` and `/tmp/account1` checks - 🔁 add `SUMMONER_SMOKE_RESET` toggle for local development
this squashes two related commits from development: --- ci: ⭐ `SIGINT` will also kill cometbft, pd, summonerd when running this locally in a development shell, i found that `ctrl+c` would not terminate the other running processes. this would stop the shell script, but logs from pd and cometbft would continue being printed to the screen. moreover, after stopping that shell, `ps` showed me that these processes were left running, causing subsequent runs of the summonerd smoke test to fail due to ports already being bound. this adds `SIGINT` to the list of signals that will bring down the cometbft, pd, and summonerd processes. this way, `ctrl+c` will properly clean things up. --- ci: ➕ don't clobber smoke test's child pids fixes #4370. this makes the list of process id's provided to `kill -9` when a SIGEXIT or SIGINT signal is received an _additive_ list. prior, we were clobbering this list when the phase1 and phase2 steps were run. now, we append a new id to the list at each point we set a signal trap.
cratelyn
force-pushed
the
kate/local-summoner-test-running-tweaks
branch
from
269ecc7 to
3998da3
Compare
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
describe your changes
this provides some small changes needed for running the summoner smoke test locally.
issue ticket number and link
fixes #4321.
fixes #4370.
related to #4319.
checklist before requesting a review
if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason: