Skip to content

Refactor some Dash-specific wait_for* functions in tests#3122

Merged
UdjinM6 merged 3 commits intodashpay:developfrom
UdjinM6:refactorsomewait
Oct 2, 2019
Merged

Refactor some Dash-specific wait_for* functions in tests#3122
UdjinM6 merged 3 commits intodashpay:developfrom
UdjinM6:refactorsomewait

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Sep 29, 2019

Refactoring only, should be no changes in behaviour. Ignoring wait_for_instantlock here because its behaviour must be changed.

@UdjinM6 UdjinM6 added this to the 14.1 milestone Sep 29, 2019
PastaPastaPasta
PastaPastaPasta requested changes Sep 29, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

scripted diff fails

~/workspace/dash$ ./contrib/devtools/commit-script-check.sh develop..HEAD
Running script for: d8b9c6bde97ff09c33126173459296a8c6e759c2
sed -i 's/wait_for_chainlock_tip_all_nodes\(/wait_for_chainlocked_tip_all_nodes\(/g' test/functional/*.py
sed -i 's/wait_for_chainlock_tip\(/wait_for_chainlocked_tip\(/g' test/functional/*.py
sed -i 's/wait_for_chainlock\(/wait_for_chainlocked_block\(/g' test/functional/*.py
sed -i 's/wait_for_chainlock /wait_for_chainlocked_block /g' test/functional/*.py
sed: -e expression #1, char 75: Unmatched ( or \(
sed: -e expression #1, char 55: Unmatched ( or \(
sed: -e expression #1, char 53: Unmatched ( or \(

Ran on Ubuntu 16.04

@UdjinM6 UdjinM6 force-pushed the refactorsomewait branch 2 times, most recently from 321f138 to 321fe82 Compare September 29, 2019 10:29
@UdjinM6 UdjinM6 mentioned this pull request Sep 29, 2019
Merged
@UdjinM6 UdjinM6 force-pushed the refactorsomewait branch 2 times, most recently from 0424795 to 321fe82 Compare September 29, 2019 14:11
@UdjinM6
Copy link
Author

UdjinM6 commented Sep 29, 2019

This should be fixed now.

On a side note though, it looks like this thing https://github.com/dashpay/dash/blob/develop/ci/build_src.sh#L15 doesn't really work (and I'm not sure why).

@codablock
Copy link

codablock commented Sep 30, 2019

@UdjinM6 I assume the TRAVIS_EVENT_TYPE check should be removed for the commit-script-check.sh to make it work. TRAVIS_EVENT_TYPE is probably only once pull_request, which is when the PR is created first. After that it becomes push as I understand it.

Maybe we should do this change in this PR so that we can also properly test it?

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Sep 30, 2019

Imo it should probably be a seperate PR that this gets rebased on, but I don't care that much.

PastaPastaPasta
PastaPastaPasta approved these changes Sep 30, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

ACK, scripted diff passes, tests pass

@UdjinM6
Copy link
Author

UdjinM6 commented Sep 30, 2019

The issue with commit-script-check.sh not running should be resolved in #3128

@UdjinM6 UdjinM6 mentioned this pull request Sep 30, 2019
Merged
codablock
codablock reviewed Oct 1, 2019
View reviewed changes
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

See inline comment

@codablock codablock mentioned this pull request Oct 1, 2019
Merged
codablock
codablock previously approved these changes Oct 1, 2019
View reviewed changes
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

utACK

PastaPastaPasta
PastaPastaPasta previously approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK

UdjinM6 added 3 commits October 1, 2019 17:18
@UdjinM6
scripted-diff: Rename wait_for_chainlock* test functions
e68cdf2 
-BEGIN VERIFY SCRIPT-
sed -i 's/wait_for_chainlock_tip_all_nodes(/wait_for_chainlocked_tip_all_nodes(/g' test/functional/*.py
sed -i 's/wait_for_chainlock_tip(/wait_for_chainlocked_tip(/g' test/functional/*.py
sed -i 's/wait_for_chainlock(/wait_for_chainlocked_block(/g' test/functional/*.py
sed -i 's/wait_for_chainlock /wait_for_chainlocked_block /g' test/functional/*.py
-END VERIFY SCRIPT-
@UdjinM6
Move wait_for_*chainlock* functions from individual tests to DashTe…
08a67cb 
…stFramework
@UdjinM6
Use wait_until in most Dash-specific wait_for* functions instead …
ee8fd7c 
…of custom timers
@UdjinM6 UdjinM6 dismissed stale reviews from PastaPastaPasta and codablock via ee8fd7c October 1, 2019 14:19
@UdjinM6
Copy link
Author

UdjinM6 commented Oct 1, 2019 •
edited
Loading

Rebased on develop to make sure it merges cleanly after #3123 and also to trigger a scripted diff check after #3128 once again just in case.

PastaPastaPasta
PastaPastaPasta approved these changes Oct 1, 2019
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

ACK, scripted diff passes as well as tests pass

codablock
codablock approved these changes Oct 1, 2019
View reviewed changes
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

re-utACK

@UdjinM6 UdjinM6 merged commit 737ac96 into dashpay:develop Oct 2, 2019
@UdjinM6 UdjinM6 deleted the refactorsomewait branch November 26, 2020 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

+1 more reviewer

@codablock codablock codablock approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

15

Development

Successfully merging this pull request may close these issues.

3 participants

@UdjinM6 @codablock @PastaPastaPasta