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 test to assert macro hygiene for Encode and Decode derives #293

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

Robbepop
Copy link
Contributor

Uses the #![no_implicit_prelude] attribute to add UI tests to guard Encode and Decode derives against macro hygiene mistakes.

This way the PR found a minor macro hygiene bug in Decode and fixes it.
Also fixed a minor bug with MaxEncodedLen test.

@gui1117
Copy link
Contributor

gui1117 commented Sep 28, 2021

miri is failing because of a false positive #288 (comment)
I think you can remove the miri test

tests/scale_codec_ui.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <[email protected]>
@Robbepop
Copy link
Contributor Author

@bkchr applied your suggestion. I just copied the code and comment from the other UI test.

@thiolliere what do you mean by remove miri test? It is failing in another crate so I think it is okay to merge the PR, right?

Could we do a minor release after this? It would be nice to have the latest changes in dependencies such as ink! that also has tests for no-implicit-prelude but fails due to parity-scale-codec.

@gui1117
Copy link
Contributor

gui1117 commented Sep 28, 2021

@thiolliere what do you mean by remove miri test? It is failing in another crate so I think it is okay to merge the PR, right?

the CI is failing, I think if we keep a ci test failing for a false positive we will never look at it again and we will miss in case another error is raised.

By removing the miri test I mean this:

diff --git a/src/bit_vec.rs b/src/bit_vec.rs
index 1beca32..35d3123 100644
--- a/src/bit_vec.rs
+++ b/src/bit_vec.rs
@@ -191,6 +191,10 @@ mod tests {
        }
 
        #[test]
+       // Flaky test due to:
+       // * https://github.com/bitvecto-rs/bitvec/issues/135
+       // * https://github.com/rust-lang/miri/issues/1866
+       #[cfg(not(miri))]
        fn bitvec_u32() {
                for v in &test_data!(u32) {
                        let encoded = v.encode();

…aritytech/parity-scale-codec into robin-add-test-for-no-implicit-prelude
@Robbepop
Copy link
Contributor Author

The miri test is removed now. Ready to merge. :)

@gui1117 gui1117 merged commit 7a845c2 into master Sep 28, 2021
@gui1117 gui1117 deleted the robin-add-test-for-no-implicit-prelude branch September 28, 2021 15:38
@gui1117
Copy link
Contributor

gui1117 commented Sep 28, 2021

I don't mind another release, we can do another PR which update the changelog and version of parity_scale_codec.
Actually we can do a patch IMO 2.3.1

@Robbepop
Copy link
Contributor Author

Actually we can do a patch IMO 2.3.1

👍

I don't mind another release, we can do another PR which update the changelog and version of parity_scale_codec.

Are you doing that or shall I?

@gui1117
Copy link
Contributor

gui1117 commented Sep 28, 2021

I'm not doing currently, you can do

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.

3 participants