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

Use static JSON in messages crate tests #822

Open
bobozaur opened this issue May 2, 2023 · 7 comments
Open

Use static JSON in messages crate tests #822

bobozaur opened this issue May 2, 2023 · 7 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@bobozaur
Copy link
Contributor

bobozaur commented May 2, 2023

Some corners were cut when developing the tests for the messages crate during its refactor in the sense that the JSON was being composed off of already tested structs or fields.

It would be better to use plain text JSON to ensure that any API change won't slip by the testing suite.

@bobozaur bobozaur added the good first issue Good for newcomers label May 2, 2023
@SumantxD
Copy link

SumantxD commented May 5, 2023

@bobozaur i would like to work on this issue can you help me navigate it in the project directory
Thank You.

@bobozaur
Copy link
Contributor Author

bobozaur commented May 8, 2023

@bobozaur i would like to work on this issue can you help me navigate it in the project directory Thank You.

Not sure what you mean. This is about most tests in msg_fields module (might be others in other modules).

The ones about msg_type deserialization are fine, but the tests that are about deserializing an actual message with fields should ideally use static messages instead of serializing certain types to avoid the situation where both the serialization and the deserialization are incorrect and thus the test passes but the result is incorrect.

@tech-bash
Copy link
Contributor

I would like to try this out.

@bobozaur
Copy link
Contributor Author

bobozaur commented May 22, 2023

I would like to try this out.

Have at it :). Let me know if you need more information. The overall goal is simple: to replace json in tests with static JSON strings (or just static strings wrapped in the json!() macro).

As an example:

        let expected = json!({
            "credentials~attach": content.credentials_attach,
            "~thread": decorators.thread
        });

Would become something like:

        let expected = json!({
            "credentials~attach": {
                "field1": "value1",
                "field2": "value2",
            },
            "~thread": {
                "field_1": "value1",
                "field_2": "value2",
            }
        });

This way, if the inner structure of say, the Thread type changes in a breaking way, we'd be more aware and the tests aren't going to just pass, as right now we're deserializing something we just serialized.

@tech-bash
Copy link
Contributor

Alright, thanks for the explanation!

@tech-bash
Copy link
Contributor

In the above example you have set ~thread to two same names i.e field1 so is there any specific reason behind it or as I am thinking that, the other value could be field2 as well!

@bobozaur
Copy link
Contributor Author

bobozaur commented Jun 1, 2023

In the above example you have set ~thread to two same names i.e field1 so is there any specific reason behind it or as I am thinking that, the other value could be field2 as well!

Ah, my bad. Edited. They are supposed to be different fields, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants