test: check assumeutxo file for changed outpoint index + de-duplications

fjahr commented at 4:37 pm on October 17, 2023: contributor

Also doing some de-duplications. I kept the second commit separate for now as I am not 100% if this is overdoing it and makes it harder to reason about. But it also makes it easier to add more cases where we change more data.

DrahtBot commented at 4:37 pm on October 17, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28659 (assumeutxo, rpc: Add ‘start’ parameter to loadtxoutset by hernanmarino)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Oct 17, 2023

maflcko commented at 11:29 am on October 19, 2023: member

nit: duplicate is written with p

fanquake renamed this:
~~test: check assumeutxo file for changed outpoint index + de-dublications~~
test: check assumeutxo file for changed outpoint index + de-duplications
on Oct 19, 2023

in test/functional/feature_assumeutxo.py:75 in 0ffc3ab55e outdated

69@@ -70,6 +70,10 @@ def test_invalid_snapshot_scenarios(self, valid_snapshot_path):
70             valid_snapshot_contents = f.read()
71         bad_snapshot_path = valid_snapshot_path + '.mod'
72 
73+        def expected_log(msg):
74+            with self.nodes[1].assert_debug_log([msg]):
75+                assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", self.nodes[1].loadtxoutset, bad_snapshot_path)

maflcko commented at 11:31 am on October 19, 2023:

Not sure about this. Won’t this hard-code make it harder to check against the actual error message, if it is provided in the future.

fjahr commented at 6:20 pm on October 19, 2023:

You mean the RPC error message, right? Unless there is already a PR that conflicts with this, isn’t it better to hardcode it in one place rather than many, i.e. following DRY? When there are changes on the RPC this will certainly evolve, maybe by removing the helper function or maybe by adding the RPC error message as an additional param to the helper.

maflcko commented at 8:02 pm on October 19, 2023:

In any case, there is already one place right below where you are not using the helper. I think it would be better to either use the helper everywhere or not at all.

fjahr commented at 12:36 pm on October 20, 2023:

Alright, I adapted it so it the help is also used for that first case.

glozow added this to the milestone 26.0 on Oct 20, 2023

fjahr force-pushed on Oct 20, 2023

test: check au file with changed outpoint index 0a576d62fe

test: De-dublicate/optimize assumeutxo test for further extensions d3223685b1

fjahr force-pushed on Oct 20, 2023

fjahr commented at 12:40 pm on October 20, 2023: contributor

Addressed @maflcko ’s feedback and rebased.

maflcko commented at 1:50 pm on October 20, 2023: member

lgtm ACK d3223685b1bb3fb4b4626d2afe4bf753e04f7b0a

DrahtBot added the label CI failed on Oct 20, 2023

maflcko commented at 2:45 pm on October 20, 2023: member

fuzz timeout can be ignored for now

achow101 commented at 8:08 pm on October 20, 2023: member

ACK d3223685b1bb3fb4b4626d2afe4bf753e04f7b0a

achow101 merged this on Oct 20, 2023

achow101 closed this on Oct 20, 2023

Frank-GER referenced this in commit 9ade1a67dc on Oct 21, 2023

bitcoin locked this on Oct 19, 2024

test: check assumeutxo file for changed outpoint index + de-duplications #28669

Code Coverage

Reviews

Conflicts