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: improvements for debugging integration tests #5402

Merged

Conversation

brianp
Copy link
Contributor

@brianp brianp commented May 23, 2023

Description

This PR does a small handful of refactoring to help when debugging the integration tests:

  • Better use of temporary directories for base node data
  • Print base node identity files
  • Better use of temporary directories for wallet data
  • Move logs from each test run into the last run directory for easier to search and more isolated logs
  • Fix 2 features so they'll run now
  • Stop using in memory databases for DHT, it's possible for their reference to be dropped and the DHT tables to be wiped causing weird behaviour in the tests
  • Stop manual cleaning of the World after a test runs, there is no need to do this, and it produced undesired behavior when printing panic data
  • Miner was attempting to connect to Esme, ensure it stays on the localnetwork

Motivation and Context

The suite can be difficult to debug, hopefully this makes it easier

How Has This Been Tested?

Locally

What process can a PR reviewer use to test or verify this change?

See if it passes CI

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@brianp brianp force-pushed the fix-node-dht-tables-in-tests branch from a30a8b2 to bd1a6b0 Compare May 23, 2023 15:04
@brianp brianp requested a review from SWvheerden May 23, 2023 15:05
stringhandler
stringhandler previously approved these changes May 23, 2023
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Nice

@ghpbot-tari-project ghpbot-tari-project added the P-acks_required Process - Requires more ACKs or utACKs label May 23, 2023
@stringhandler stringhandler enabled auto-merge May 23, 2023 15:09
@github-actions
Copy link

github-actions bot commented May 23, 2023

Test Results (CI)

1 147 tests  ±0   1 147 ✔️ ±0   10m 0s ⏱️ -37s
     37 suites ±0          0 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 6a5dc8f. ± Comparison against base commit f24784f.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 23, 2023

Test Results (Integration tests)

29 tests  +29   29 ✔️ +29   16m 57s ⏱️ + 16m 57s
12 suites +12     0 💤 ±  0 
  2 files   +  2     0 ±  0 

Results for commit 6a5dc8f. ± Comparison against base commit f24784f.

♻️ This comment has been updated with latest results.

SWvheerden
SWvheerden previously approved these changes May 24, 2023
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

ACK

brianp added 10 commits May 24, 2023 08:33
In memory databases are impossible to debug after the fact. Using file
db's allows us to inspect them when we get strange results, and
simulates real node use better.
Many of the files used for nodes and wallets were being spread accross
multiple temp directories. This commit attempts to alleviate that and
moves to using temporay, but persisted directories we can debug after
the test run.
A logger was added to print the world client struct after a failure has
occured. This was showing most collection as empty due to the fact we
were purposfully clearing it after each singular test run. The purpose
of clearing it was to ensure no crossover or spillage from one test to
another but this isn't a real problem with cucumber. The world object is
cloned new into every test run and isn't shared, so we don't need to
clean it out.
Right now the logs will continue to ammend a single file. Sometimes this
is bothersome when wanting to debug the last one, as the file may be
quite large. This commit will move the logs into the temp directory of
the last run isolating that runs logs for easier debugging.
The nodes weren't communicating yet when the first block becomes mined
and as a result the LAG1 node never saw the block via propagation. Give
just a bit more time for communication to be open before mining ensuring
the nodes will gossip.
Validation is being run at import now which means the invalid UTXO's
are never reflected in the balance. Remove the balance check step and
carry on.
auto-merge was automatically disabled May 24, 2023 06:37

Head branch was pushed to by a user without write access

@brianp brianp dismissed stale reviews from SWvheerden and stringhandler via 6a5dc8f May 24, 2023 06:37
@brianp brianp force-pushed the fix-node-dht-tables-in-tests branch from bd1a6b0 to 6a5dc8f Compare May 24, 2023 06:37
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 24, 2023
@SWvheerden SWvheerden merged commit 8631bc2 into tari-project:development May 24, 2023
@brianp brianp deleted the fix-node-dht-tables-in-tests branch May 24, 2023 10:16
SWvheerden pushed a commit that referenced this pull request May 25, 2023
Description
---
This PR does a small handful of refactoring to help when debugging the
integration tests:
- Better use of temporary directories for base node data
- Print base node identity files
- Better use of temporary directories for wallet data
- Move logs from each test run into the last run directory for easier to
search and more isolated logs
- Fix 2 features so they'll run now
- Stop using in memory databases for DHT, it's possible for their
reference to be dropped and the DHT tables to be wiped causing weird
behaviour in the tests
- Stop manual cleaning of the World after a test runs, there is no need
to do this, and it produced undesired behavior when printing panic data
- Miner was attempting to connect to Esme, ensure it stays on the
localnetwork

Motivation and Context
---
The suite can be difficult to debug, hopefully this makes it easier

How Has This Been Tested?
---
Locally

What process can a PR reviewer use to test or verify this change?
---
See if it passes CI

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants