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

Added tests for map coordinates. #776

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

borondics
Copy link
Member

No description provided.

@markotoplak
Copy link
Collaborator

I am fine with this precise checking if it passes. Otherwise, it this case, where they should be mostly rounded to the integer, assertAlmostEqual with the correct parameters would be preferred.

What I do not like is that these tests only test two pixels in the first line or column (do not know which is which). The more interesting cases are the ones somewhere in the middle of the data.

@borondics
Copy link
Member Author

This test file is a 4x4 map so we are actually testing the middle pixels.

@markotoplak
Copy link
Collaborator

Yes, you are testing middle pixels on the first row/column. Please test middle pixes on middle row/column. :)

@borondics
Copy link
Member Author

How about a test like this? All the coordinates... :)

@markotoplak
Copy link
Collaborator

Your test was wrongly structured. map_x and map_y were not the expected outputs but their rounded-down values (your chandex x.9999 to x.0000). This was very confusing and tests should not be written like this. Instead, you should have rounded the metas to the nearest int and compared after.

I fixed this and simplified a bit.

@borondics
Copy link
Member Author

Your solution is much nicer!

However, while confusing, I think the int() or round() difference is not important as what you are testing is code stability, not real things, right? In any case, thanks for the improvement.

@markotoplak
Copy link
Collaborator

markotoplak commented Nov 27, 2024

However, while confusing, I think the int() or round() difference is not important as what you are testing is code stability, not real things, right?

Tests should always be as real as you can make them. Ideally, you write them them knowing what the output is (should not fit inputs so that the output matches). And something being confusing is definitely a maintenance problem.

@markotoplak markotoplak merged commit 76a4403 into Quasars:master Nov 27, 2024
14 checks passed
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.

2 participants