integration testing scripts #64

Merged
jeffreypicard merged 8 commits from integration_testing into master 2022-10-04 19:25:44 +02:00
jeffreypicard commented 2022-09-21 12:24:33 +02:00 (Migrated from github.com)

some scripts for integration testing and a docker file for an action. Still need to figure out how to properly run a more realistic version in ci.

some scripts for integration testing and a docker file for an action. Still need to figure out how to properly run a more realistic version in ci.
moodyjon (Migrated from github.com) requested changes 2022-09-21 20:09:33 +02:00
moodyjon (Migrated from github.com) left a comment

Also, this appears to fall back on /mnt/d/data/snapshot_1072108/lbry-rocksdb/ which I still don't know how to obtain, and I don't know how the GitHub runner gets a copy. I can live without it for now, but it would be great to be able to run locally.

Also, this appears to fall back on `/mnt/d/data/snapshot_1072108/lbry-rocksdb/` which I still don't know how to obtain, and I don't know how the GitHub runner gets a copy. I can live without it for now, but it would be great to be able to run locally.
moodyjon (Migrated from github.com) commented 2022-09-21 19:56:16 +02:00

This is missing herald launch, and calling integration_tests.sh.

This is missing herald launch, and calling integration_tests.sh.
moodyjon (Migrated from github.com) commented 2022-09-21 19:48:48 +02:00

Suggest substituting the value here, and eliminating CHUNK_TEST_RES as it's not reused by other tests.

Suggest substituting the value here, and eliminating `CHUNK_TEST_RES` as it's not reused by other tests.
moodyjon (Migrated from github.com) commented 2022-09-21 20:02:27 +02:00

Greater than or equal to (-ge). At least for the one test using this, you wouldn't want GOT=0 to pass.

Greater than or equal to (`-ge`). At least for the one test using this, you wouldn't want GOT=0 to pass.
@ -0,0 +164,4 @@
--data '{"id": 1, "method": "blockchain.scripthash.get_mempool", "params":[{"scripthash": "bGqWuXRVm5bBqLvLPEQQpvsNxJ5ubc6bwN"}]}'
| jq .error | sed 's/"//g'
EOM
WANT="encoding/hex: invalid byte: U+0047 'G'"
moodyjon (Migrated from github.com) commented 2022-09-21 19:41:45 +02:00

Yeah.... the scripthash ones are tricky. I don't have an example of constructing one. Lbry-sdk does not send them.

I haven't figured it out yet, but see:
https://github.com/btcsuite/btcd/search?p=1&q=AddressScriptHash&type=Code

Yeah.... the scripthash ones are tricky. I don't have an example of constructing one. Lbry-sdk does not send them. I haven't figured it out yet, but see: https://github.com/btcsuite/btcd/search?p=1&q=AddressScriptHash&type=Code
moodyjon (Migrated from github.com) reviewed 2022-09-22 19:02:40 +02:00
@ -0,0 +164,4 @@
--data '{"id": 1, "method": "blockchain.scripthash.get_mempool", "params":[{"scripthash": "bGqWuXRVm5bBqLvLPEQQpvsNxJ5ubc6bwN"}]}'
| jq .error | sed 's/"//g'
EOM
WANT="encoding/hex: invalid byte: U+0047 'G'"
moodyjon (Migrated from github.com) commented 2022-09-22 19:02:40 +02:00

Just found this. Purports to convert addr to scripthash:
https://github.com/checksum0/go-electrum/blob/b862ac442cf9/electrum/address.go#L14

Just found this. Purports to convert addr to scripthash: https://github.com/checksum0/go-electrum/blob/b862ac442cf9/electrum/address.go#L14
jeffreypicard commented 2022-09-22 19:38:43 +02:00 (Migrated from github.com)

This definitely requires a local and complete snapshot of the current db at this point. Which requires running your own hub to have in general. Another big piece of what else I'm working on, but not something that in general will be documented here. Getting this running as part of a ci runner we do need to figure out, there's a dependency on hundreds of gigabytes of data to do a "real" smoke test on, so it's not clear the best way to do this.

This definitely requires a local and complete snapshot of the current db at this point. Which requires running your own hub to have in general. Another big piece of what else I'm working on, but not something that in general will be documented here. Getting this running as part of a ci runner we do need to figure out, there's a dependency on hundreds of gigabytes of data to do a "real" smoke test on, so it's not clear the best way to do this.
jeffreypicard (Migrated from github.com) reviewed 2022-09-24 08:04:16 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-24 08:04:16 +02:00

I think I've just transposed the WANT and GOT, but the -gt is correct. I want a number greater than 0.

I think I've just transposed the WANT and GOT, but the `-gt` is correct. I want a number greater than 0.
jeffreypicard (Migrated from github.com) reviewed 2022-09-24 08:28:09 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-24 08:28:09 +02:00

Actually, nvm, you are right.

Actually, nvm, you are right.
moodyjon (Migrated from github.com) requested changes 2022-09-27 18:58:16 +02:00
moodyjon (Migrated from github.com) commented 2022-09-27 18:22:05 +02:00

This should be before db.NewIteratorCF() and possibly before grocksdb.NewDefaultReadOptions().

This should be before `db.NewIteratorCF()` and possibly before `grocksdb.NewDefaultReadOptions()`.
moodyjon (Migrated from github.com) commented 2022-09-27 18:23:25 +02:00

This whole block (if opts.DB != nil {...) should be after it.Close() and possibly after ro.Destroy().

This whole block (`if opts.DB != nil {...`) should be after `it.Close()` and possibly after `ro.Destroy()`.
moodyjon (Migrated from github.com) commented 2022-09-27 18:34:03 +02:00

If a thread is waiting on ShutdownChan in db.Shutdown() and holding the lock, this line (364) can't be reached. Send the done message just before locking, so db.Shutdown() can proceed.

If a thread is waiting on `ShutdownChan` in `db.Shutdown()` and holding the lock, this line (364) can't be reached. Send the done message just **before** locking, so `db.Shutdown()` can proceed.
moodyjon (Migrated from github.com) commented 2022-09-27 18:27:27 +02:00

Position [0] is DoneChan, and position [1] is ShutdownChan. I think you mean to send to ShutdownChan, then receive from DoneChan.

I think this scheme could be simplified by having a single bidirectional chan. Send the shutdown message, then receive the done message on the same chan.

Position `[0]` is `DoneChan`, and position `[1]` is `ShutdownChan`. I think you mean to send to `ShutdownChan`, then receive from `DoneChan`. I think this scheme could be simplified by having a single bidirectional chan. Send the shutdown message, then receive the done message on the same chan.
moodyjon (Migrated from github.com) commented 2022-09-27 18:44:16 +02:00

My first thought: Yes of course. But...

...on investigating how this works, ShutdownChan works to stop RunDetectChanges(). Since that's a source of activity that may spawn new iterators, it's a good idea to stop that first so no new iterators are created. Also, consider waiting to receive the done message (<- db.DoneChan) before waiting on the iterators to be done.

The real grocksdb shutdown stuff that would break running iterators probably lies in db.Cleanup().

My first thought: Yes of course. But... ...on investigating how this works, `ShutdownChan` works to stop `RunDetectChanges()`. Since that's a source of activity that may spawn new iterators, it's a good idea to stop that first so no new iterators are created. Also, consider waiting to receive the done message (`<- db.DoneChan`) before waiting on the iterators to be done. The real grocksdb shutdown stuff that would break running iterators probably lies in `db.Cleanup()`.
jeffreypicard (Migrated from github.com) reviewed 2022-09-28 13:14:54 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-28 13:14:53 +02:00

II agree i used the wrong channels. I don't think you can really do the bi-directional thing though. It would create a new race conditional where I would send the notification then immediately read it off before the thread it was intended for did.

II agree i used the wrong channels. I don't think you can really do the bi-directional thing though. It would create a new race conditional where I would send the notification then immediately read it off before the thread it was intended for did.
jeffreypicard (Migrated from github.com) reviewed 2022-09-28 13:18:13 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-28 13:18:13 +02:00

I agree.

I agree.
jeffreypicard (Migrated from github.com) reviewed 2022-09-28 13:18:20 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-28 13:18:20 +02:00

sure

sure
jeffreypicard (Migrated from github.com) reviewed 2022-09-28 13:24:06 +02:00
jeffreypicard (Migrated from github.com) commented 2022-09-28 13:24:05 +02:00

good call

good call
moodyjon (Migrated from github.com) reviewed 2022-09-28 14:01:46 +02:00
moodyjon (Migrated from github.com) commented 2022-09-28 14:01:45 +02:00

OK

OK
moodyjon (Migrated from github.com) requested changes 2022-09-28 14:17:40 +02:00
moodyjon (Migrated from github.com) commented 2022-09-28 14:03:55 +02:00

The opts.DoneChan <- struct{}{} is still inside the critical section. It should be before Lock().

Edit: Oops, I didn't notice. It is outside the critical section. But it needs to be before.

Edit Edit: It moved from after the critical section to inside in the last change I think. The code displayed above is the original form where the done message is sent after the critical section.

The `opts.DoneChan <- struct{}{}` is still inside the critical section. It should be before `Lock()`. Edit: Oops, I didn't notice. It *is* outside the critical section. But it needs to be *before*. Edit Edit: It moved from *after* the critical section to *inside* in the last change I think. The code displayed above is the original form where the done message is sent after the critical section.
moodyjon (Migrated from github.com) commented 2022-09-28 14:16:56 +02:00

I don't see anything using WithDB, so the new logic never becomes active. Perhaps make NewIterateOptions() take db as an argument?

I don't see anything using `WithDB`, so the new logic never becomes active. Perhaps make `NewIterateOptions()` take `db` as an argument?
jeffreypicard (Migrated from github.com) reviewed 2022-10-03 11:41:10 +02:00
jeffreypicard (Migrated from github.com) commented 2022-10-03 11:41:10 +02:00

Good call. I made the first change, and did changed the existing NewIterateOptions to use WithDB

Good call. I made the first change, and did changed the existing NewIterateOptions to use WithDB
moodyjon (Migrated from github.com) approved these changes 2022-10-03 15:23:19 +02:00
moodyjon (Migrated from github.com) commented 2022-10-03 15:23:10 +02:00

One more tiny thing... At this point, it might be that db.Shutdown() has already been called. Because we are not yet in the map, the interruptRequested() func can't tell us anything. There should be a bool in DB that tells us Shutdown() has been called and no more registrations are being accepted. Set it in Shutdown() and check it here, exiting early if needed.

One more tiny thing... At this point, it might be that db.Shutdown() has already been called. Because we are not yet in the map, the interruptRequested() func can't tell us anything. There should be a bool in DB that tells us Shutdown() has been called and no more registrations are being accepted. Set it in Shutdown() and check it here, exiting early if needed.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/herald.go#64
No description provided.