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

QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core" #30308

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jun 19, 2024

Followup to #29144

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 19, 2024

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 kevkevinpal, tdb3

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

@DrahtBot DrahtBot added the Tests label Jun 19, 2024
@kevkevinpal
Copy link
Contributor

ACK 197b540

we're doing this in these files aswell so seems fine to me

test/functional/wallet_multiwallet.py 
test/functional/tool_wallet.py 
test/functional/interface_bitcoin_cli.py 
test/functional/feature_filelock.py

@kevkevinpal
Copy link
Contributor

I see we're doing something similar in ./test/functional/wallet_backwards_compatibility.py

node_v17.assert_start_raises_init_error(["-wallet=w3"], "Error: Error loading w3: Wallet requires newer version of Bitcoin Core")

and

node_v16.assert_start_raises_init_error([f"-wallet={wallet_name}"], f"Error: Error loading {wallet_name}: Wallet requires newer version of Bitcoin Core")

not sure if you want to update here aswell in this PR

Copy link
Contributor

@tdb3 tdb3 left a comment

Choose a reason for hiding this comment

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

ACK 197b540
grepped for other instances of Bitcoin Core in test/functional/ and didn't see any beyond what @kevkevinpal identified.

@fanquake
Copy link
Member

not sure if you want to update here aswell in this PR

@luke-jr can you update this PR to include the additional changes.

@maflcko
Copy link
Member

maflcko commented Jun 20, 2024

previous releaseas are hardcoded to be Bitcoin Core, so no need to change that

I wonder if one CI task should modify the PACKAGE_NAME to be something else, e.g. Bitcoin Core CI-build?

@fanquake
Copy link
Member

previous releaseas are hardcoded to be Bitcoin Core, so need to change that

It can be changed just for consistency, and to prevent similar issues in future when devs copy-paste code.

@luke-jr
Copy link
Member Author

luke-jr commented Jun 21, 2024

It can be changed just for consistency, and to prevent similar issues in future when devs copy-paste code.

Changing it would break that test.

I don't think there's anything more to do here at the moment. (CI is a good idea, but seems suited to a different PR)

@fanquake
Copy link
Member

Changing it would break that test.

Not in our repository though? In any case, this could be revisited if someone follows up with a CI change.

@fanquake fanquake merged commit a57da5e into bitcoin:master Jun 24, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants