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

[Telemetry] proptest for parsers.rs #203

Merged
merged 19 commits into from
Apr 27, 2020
Merged

Conversation

ronanM
Copy link
Contributor

@ronanM ronanM commented Apr 26, 2020

I have added proptest for parsing BootMessage (but I don't understood the device_id logic).

If you found that useful, i could continue with other messages.

@BlackYoup
Copy link
Collaborator

Hi!

I didn't know proptest, this seems nice. From what I understand, it will test multiple scenarios based on the format you give and will change parts of it each time?

but I don't understood the device_id logic

I may be mistaken but each makair device will have its own ID which is given in the device_id field. You can find more information in this issue: #120

I'll let @dsferruzza rule on this one but it seems good for me (maybe just remove the commented // println!("{:?}",&msg);)

Copy link
Member

@dsferruzza dsferruzza left a comment

Choose a reason for hiding this comment

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

I have few minor changes to request but otherwise this looks good to me!

src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
ronanM and others added 7 commits April 26, 2020 18:15
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
@dsferruzza
Copy link
Member

@ronanM I just noticed that clippy rises several warnings; could you have a look?

I think you just need to have clippy installed and run cargo clippy --all-targets --all-features.

@ronanM
Copy link
Contributor Author

ronanM commented Apr 26, 2020

I think that all TelemetryMessage messages are tested.

Copy link
Member

@dsferruzza dsferruzza left a comment

Choose a reason for hiding this comment

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

Cool! I found other minor glitches, almost there!

Also is this indentation intentional?
image

I can't get rustfmt to help us here, I guess this is because most of the code is inside the proptest! macro(?)

src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
src/software/telemetry/src/parsers.rs Outdated Show resolved Hide resolved
ronanM and others added 5 commits April 26, 2020 21:59
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
Co-Authored-By: David Sferruzza <david.sferruzza@gmail.com>
@dsferruzza dsferruzza merged commit 6113917 into makers-for-life:master Apr 27, 2020
@dsferruzza
Copy link
Member

Thanks @ronanM!

@ronanM ronanM deleted the proptest branch April 27, 2020 09:58
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