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

Clean up IO tests to use hex or constants in comparisons #29

Open
lawik opened this issue Jul 26, 2019 · 4 comments
Open

Clean up IO tests to use hex or constants in comparisons #29

lawik opened this issue Jul 26, 2019 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@lawik
Copy link
Collaborator

lawik commented Jul 26, 2019

Currently we have things like:

assert get_border_command() == [send_command: {60, 51}]

While the command being sent is provided in hex:

write_command(state, 0x3C, border_data)

0x3C is output as 60 and that's why things are this way right now. We should probably change it so that the connection remains clear.

@lawik lawik added enhancement New feature or request good first issue Good for newcomers labels Jul 26, 2019
@lawik
Copy link
Collaborator Author

lawik commented Jul 26, 2019

This would benefit from #30

@nyaray
Copy link
Collaborator

nyaray commented Jul 26, 2019

How would this benefit from #30?

I'd say we want numbers to remain literals and not references in our tests. If our tests and production code refer to the same values, we are not testing that the right values are being passed to the devices, making the tests weaker.

Being consistent in how we represent our integers is all fine and well, but going beyond that doesn't have clear benefits to me.

@lawik
Copy link
Collaborator Author

lawik commented Jul 26, 2019

Okay, that's fair. I agree that literals are probably the way to go. But I think module attributes in the test suite would be beneficial to clarity and reduce the risk of a hexadecimal number being replaced because it is identical but used for a different purpose in a different context. I think that risk is reduced with using module attributes in the test module as opposed to repeating the literals in every test.

What do you think about that approach?

@nyaray
Copy link
Collaborator

nyaray commented Jul 26, 2019

Sounds good!

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

No branches or pull requests

2 participants