Skip to content

Conversation

@wischli
Copy link
Contributor

@wischli wischli commented Aug 6, 2021

No ticket

In #231 none of the CI tests failed but cargo test was broken for the did pallet. This should be fixed by #235. However, in #235 we stumbled into the same error. Thus, I think we should add a separate `cargo test step to the CI as proposed here.

Checklist:

  • I have verified that the code works
    • No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
  • I have verified that the code is easy to understand
  • This PR does not introduce new custom types
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@wischli wischli requested a review from weichweich August 6, 2021 07:28
@wischli
Copy link
Contributor Author

wischli commented Aug 6, 2021

CI failure proves my point of adding this to the CI. They should be fixed after merging #235 as mentioned above.

@ntn-x2
Copy link
Contributor

ntn-x2 commented Aug 6, 2021

Should we add --all-targets also to cargo test? I guess we could go for cargo test --all --all-targets and cargo test --all --all-features --all-targets?

@wischli
Copy link
Contributor Author

wischli commented Aug 6, 2021

Should we add --all-targets also to cargo test? I guess we could go for cargo test --all --all-targets and cargo test --all --all-features --all-targets?

Yeah, that sounds good.

Copy link
Contributor

@weichweich weichweich left a comment

Choose a reason for hiding this comment

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

my only objection is that this will increase the load on the server. We have 2 concurrent jobs at the moment. Maybe we should increase that to 3 or 4. 🤔

@wischli wischli merged commit b1431f4 into develop Aug 13, 2021
@wischli wischli deleted the wf-ci-add-cargo-test branch August 13, 2021 12:49
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.

4 participants