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

refactor(neard/tests): reduce boilerplate with intuitive structures #4229

Merged
merged 34 commits into from
Apr 17, 2021

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Apr 16, 2021

  • reduce boilerplate with intuitive structures: barely changing patterns repeated over and over in neard/tests/* are simplified with easy to reason about structures.
  • run_actix_until (near-actix-test-utils): cancel all active async tasks on SIGINT (ctrl+c) and panic pending ones.

• Serially execute every child that's actively running.
• Kill the parent of every child that's not
  yet running so they panic and die of shock.

Everybody Dies™ :-)
a delay on start_nodes while a SIGINT is issued\ncould
this way, the main thread (always) awaits completion of its kids

the behaviour is a lot more concise, reasonable and effective
@frol frol requested review from frol and khorolets April 16, 2021 08:31
Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

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

Overall looks goods!

neard/tests/rpc_nodes.rs Show resolved Hide resolved
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@miraclx Great job! I have a few ideas in the comments

neard/tests/run_nodes.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

👍 on turning setup code into setup data!

neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
@miraclx miraclx force-pushed the neard-tests-structured-boilerplate branch from 4a57aae to 56fba0d Compare April 16, 2021 14:43
not all tasks within async context are non-blocking

issuing a SIGINT while on a blocking task was getting ignored

stop actix with exit code 130 in the unix way 🤓
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
neard/tests/node_cluster.rs Outdated Show resolved Hide resolved
@miraclx miraclx force-pushed the neard-tests-structured-boilerplate branch from 49fbc59 to 8c9ed4a Compare April 17, 2021 16:08
@frol frol marked this pull request as ready for review April 17, 2021 19:35
@frol frol added the automerge label Apr 17, 2021
@near-bulldozer near-bulldozer bot merged commit f816029 into near:master Apr 17, 2021
@miraclx miraclx deleted the neard-tests-structured-boilerplate branch April 17, 2021 22:31
matklad added a commit to matklad/nearcore that referenced this pull request May 13, 2022
I am not sure why we were catching ctrl+c in the first place. This was
introduced in near#4229 I think. But messing up with signal handlers in
tests doesn't feel right and, indeed, prevents tests from being killed.

Test Plan
---------

Manual testing: running `cargo t -p integration-tests` and then killing
it in the middle with ctrl+c leaves the test running in the background
before this PR, and properly kills the process after.
matklad added a commit to matklad/nearcore that referenced this pull request May 13, 2022
I am not sure why we were catching ctrl+c in the first place. This was
introduced in near#4229 I think. But messing up with signal handlers in
tests doesn't feel right and, indeed, prevents tests from being killed.

Test Plan
---------

Manual testing: running `cargo t -p integration-tests` and then killing
it in the middle with ctrl+c leaves the test running in the background
before this PR, and properly kills the process after.
near-bulldozer bot pushed a commit that referenced this pull request May 13, 2022
I am not sure why we were catching ctrl+c in the first place. This was
introduced in #4229 I think. But messing up with signal handlers in
tests doesn't feel right and, indeed, prevents tests from being killed.

Test Plan
---------

Manual testing: running `cargo t -p integration-tests` and then killing
it in the middle with ctrl+c leaves the test running in the background
before this PR, and properly kills the process after.
nikurt pushed a commit that referenced this pull request May 17, 2022
I am not sure why we were catching ctrl+c in the first place. This was
introduced in #4229 I think. But messing up with signal handlers in
tests doesn't feel right and, indeed, prevents tests from being killed.

Test Plan
---------

Manual testing: running `cargo t -p integration-tests` and then killing
it in the middle with ctrl+c leaves the test running in the background
before this PR, and properly kills the process after.
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.

None yet

5 participants