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(sns): Add integration test for setting custom proposal topics #4170

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

aterga
Copy link
Member

@aterga aterga commented Feb 28, 2025

This PR adds an integration test for the scenario in which the topic is changed for a custom SNS proposal type.

< Previous PR |

@github-actions github-actions bot added the test label Feb 28, 2025
@aterga aterga changed the base branch from master to arshavir/NNS1-3581 February 28, 2025 13:05
@aterga aterga changed the title test(sns): Add integration test for SetCustomProposalTopics test(sns): Add integration test for setting custom proposal topics Feb 28, 2025
@aterga aterga marked this pull request as ready for review February 28, 2025 13:05
@aterga aterga requested a review from a team as a code owner February 28, 2025 13:05
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

If this pull request affects the behavior of any canister owned by
the Governance team, remember to update the corresponding
unreleased_changes.md file(s).

To acknowldge this reminder (and unblock the PR), dismiss this
code review by going to the bottom of the pull request page, and
supply one of the following reasons:

  1. Done.

  2. No canister behavior changes.

Base automatically changed from arshavir/NNS1-3581 to master February 28, 2025 15:41
Copy link
Contributor

@daniel-wong-dfinity-org daniel-wong-dfinity-org left a comment

Choose a reason for hiding this comment

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

I think you just forgot to rebase|merge this -> I only looked at files in the integration_tests dir. If that's all there is to look at here, no need to ask me for further review. Otherwise, please lmk what other changes are "actually" part of this PR.

..initial_function.clone()
};

// AddGenericNervousSystemFunction
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this comment can and should contain prose, since that is the standard way.

.await
.unwrap();

let proposal_id = SubmittedProposal::try_from(response)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why conversion is needed...

.unwrap();
}

// SetCustomProposalTopics
Copy link
Contributor

Choose a reason for hiding this comment

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

Prose?

Also, this is where we run the code under test, right?

url: DUMMY_URL_FOR_PROPOSALS.to_string(),
action: Some(Action::SetCustomProposalTopics(SetCustomProposalTopics {
custom_function_id_to_topic: btreemap! {
1111_u64 => Topic::ApplicationBusinessLogic,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think _u64 prefix is not needed? Doesn't seem useful, but up to you.

michael-weigelt pushed a commit that referenced this pull request Feb 28, 2025
This PR introduces a new type of SNS proposals,
`SetCustomProposalTopics`, which can be used to batch-set topics for all
custom proposals (or any non-empty subset thereof) at once.

| [Next PR](#4170) >
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.

2 participants