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

Add cargo test CI workflow #27

Merged
merged 4 commits into from
May 3, 2024
Merged

Add cargo test CI workflow #27

merged 4 commits into from
May 3, 2024

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented May 2, 2024

Issue: #14

Changes

  • Fixes a doctest which only works with feature all-types.
  • Adds a CI (using the Spacewalk's as base) that will only run test and build with all features enabled.

@gianfra-t gianfra-t linked an issue May 2, 2024 that may be closed by this pull request
@gianfra-t gianfra-t requested a review from a team May 2, 2024 16:38
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good overall, just left some nit-picky comments 👍

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 213 to 215
#[cfg_attr(feature = "all-types", doc = "```")]
#[cfg_attr(not(feature = "all-types"), doc = "```ignore")]
/// use substrate_stellar_sdk::{types::Auth, parse_stellar_type};
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a comment here on why this is necessary? Maybe we can add the explanation as a a simple comment // here instead of a doc comment without breaking the rest?

Copy link
Member

Choose a reason for hiding this comment

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

Did you check the generated docs whether the lines

/// // For this test, we require the usage of `Auth` which is only compiled with
/// // the `all-types` feature.
/// // We will ignore the test if the feature is not enabled.

are ignored? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay I misunderstood what you meant, I thought ignored on compilation. Latest push should show it like this (they just need to be in the scope of the test definition)
image

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks 🙏

Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Should be ready for merge then

@gianfra-t gianfra-t merged commit cda1a1c into polkadot-v0.9.42 May 3, 2024
1 check passed
@gianfra-t gianfra-t deleted the cargo-test-ci branch May 3, 2024 17:09
@ebma ebma mentioned this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup CI pipeline
2 participants