MISC/mcl_selftests: Do not crash if minetest.find_nodes_in_area() lies #214

erlehmann · 2021-12-15T13:27:50+01:00

erlehmann commented

2021-12-15 13:27:50 +01:00

Problem

TRACKING ISSUE: #183

I foolishly added an assertion to the code that crashed the game if the Minetest engine used to run Mineclonia had a minetest.find_nodes_in_area() implementation that would not count nodes correctly near the map border.

After sfan5 changed minetest.find_nodes_in_area() to lie about nodes with coordinate components larger than 31000 or smaller than -31000, this test crashed Mineclonia at startup with dvelopment versions of Minetest. This made several people – among them @cora and sfan5 – upset.

@cora convined me that it is wrong to have no error message. @EliasFleckenstein03 convinced me that it is wrong to use assert() for non-catastrophic bugs outside of one's own code. Therefore, I have replaced the assertion with a (hopefully helpful) debug message that is printed when the test fails.

Solution

Instead of a crash, the game now complains about the bug in the engine via minetest.debug() at startup.

Details

I removed the assert() and inserted an if statement.

Testing Steps

Start Minetest 5.4.1 with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de.
Verify that no debug message is printed at startup.
Start Minetest commit 87ab97da2a with Mineclonia commit 6f811b3cee.
Verify that the game crashes with a failed assertion in mcl_selftests.
Start Minetest commit 87ab97da2a with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de.
Verify that a debug message is printed at startup, similar to the folllowing: minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.

##### Problem TRACKING ISSUE: https://git.minetest.land/Mineclonia/Mineclonia/issues/183 I foolishly added an assertion to the code that crashed the game if the Minetest engine used to run Mineclonia had a `minetest.find_nodes_in_area()` implementation that would not count nodes correctly near the map border. After sfan5 changed `minetest.find_nodes_in_area()` to lie about nodes with coordinate components larger than 31000 or smaller than -31000, this test crashed Mineclonia at startup with dvelopment versions of Minetest. This made several people – among them @cora and sfan5 – upset. @cora convined me that it is wrong to have no error message. @EliasFleckenstein03 convinced me that it is wrong to use `assert()` for non-catastrophic bugs outside of one's own code. Therefore, I have replaced the assertion with a (hopefully helpful) debug message that is printed when the test fails. ##### Solution Instead of a crash, the game now complains about the bug in the engine via `minetest.debug()` at startup. ##### Details I removed the `assert()` and inserted an `if` statement. ##### Testing Steps 1. Start Minetest 5.4.1 with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de. 2. Verify that no debug message is printed at startup. 3. Start Minetest commit https://github.com/minetest/minetest/commit/87ab97da2ace31fdb46a88a0901ec664dd666feb with Mineclonia commit 6f811b3cee19c5c7c84232fe5ceaeb085a348df0. 4. Verify that the game crashes with a failed assertion in mcl_selftests. 5. Start Minetest commit https://github.com/minetest/minetest/commit/87ab97da2ace31fdb46a88a0901ec664dd666feb with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de. 6. Verify that a debug message is printed at startup, similar to the folllowing: `minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.`

erlehmann changed title from ~~WIP: MISC/mcl_selftests: Do not crash if minetest.find_nodes_in_area() lies~~ to MISC/mcl_selftests: Do not crash if minetest.find_nodes_in_area() lies

2021-12-15 13:36:26 +01:00

LizzyFleckenstein03 commented

2021-12-15 14:01:54 +01:00

I checked out Mineclonia/fix-selftest-crash and started my Minetest 5.4.1 installation.
No debug message was printed
I checked out Mineclonia/master and started Minetest, compiled from source on the latest commit
The game redused to start with a failed assertion as the error message
I checked out Mineclonia/fix-selftest-crash and started Minetest, compiled from source on the latest commit
I got this message: 2021-12-15 14:00:41: [Server]: minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.

Everything works as expected.

1. I checked out `Mineclonia/fix-selftest-crash` and started my Minetest 5.4.1 installation. 2. No debug message was printed 3. I checked out `Mineclonia/master` and started Minetest, compiled from source on the latest commit 4. The game redused to start with a failed assertion as the error message 5. I checked out `Mineclonia/fix-selftest-crash` and started Minetest, compiled from source on the latest commit 6. I got this message: `2021-12-15 14:00:41: [Server]: minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.` Everything works as expected.

cora commented

2021-12-15 20:27:39 +01:00

I'm not sure I like this to be honest. I would still prefer these tests only to be run when people explicitly demand them.

It should also be pointed out that the problem here isn't me or anyone else being upset. But the damn game crashing for no actual reason and with no indication of what is wrong.

I'm not sure I like this to be honest. I would still prefer these tests only to be run when people explicitly demand them. It should also be pointed out that the problem here isn't me or anyone else being upset. But the damn game crashing for no actual reason and with no indication of what is wrong.

cora commented

2021-12-15 20:29:14 +01:00

This way nothing would prevent this from happening again.

erlehmann commented

2021-12-15 20:44:04 +01:00

This way nothing would prevent this from happening again.

Me no longer writing tests containing assertions regarding anything outside of Mineclonia and you not approving them (you told me you would no longer approve of self tests that crash on engine bugs) seems to me like it would prevent this from happening again.

> This way nothing would prevent this from happening again. Me no longer writing tests containing assertions regarding anything outside of Mineclonia and you not approving them (you told me you would no longer approve of self tests that crash on engine bugs) seems to me like it would prevent this from happening again.

erlehmann commented

2021-12-15 20:45:17 +01:00

I would still prefer these tests only to be run when people explicitly demand them.

That is a separate issue though. Making the test case not crash does not take away the possibility of disabling tests.

> I would still prefer these tests only to be run when people explicitly demand them. That is a separate issue though. Making the test case not crash does not take away the possibility of disabling tests.

cora force-pushed fix-selftest-crash from 6d88a3378c to 0b5fa14041

2021-12-20 23:41:33 +01:00

Compare

cora approved these changes 2021-12-20 23:42:46 +01:00

cora left a comment

1. Start Minetest 5.4.1 with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de.
2. Verify that no debug message is printed at startup.
3. Start Minetest commit 87ab97da2a with Mineclonia commit 6f811b3cee.
4. Verify that the game crashes with a failed assertion in mcl_selftests.
5. Start Minetest commit 87ab97da2a with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de.
6. Verify that a debug message is printed at startup, similar to the folllowing: minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.

- [x] 1. Start Minetest 5.4.1 with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de. - [x] 2. Verify that no debug message is printed at startup. - [x] 3. Start Minetest commit https://github.com/minetest/minetest/commit/87ab97da2ace31fdb46a88a0901ec664dd666feb with Mineclonia commit 6f811b3cee19c5c7c84232fe5ceaeb085a348df0. - [x] 4. Verify that the game crashes with a failed assertion in mcl_selftests. - [x] 5. Start Minetest commit https://github.com/minetest/minetest/commit/87ab97da2ace31fdb46a88a0901ec664dd666feb with Mineclonia commit 7c9845ac3fece869da9725deb04a24434a5955de. - [x] 6. Verify that a debug message is printed at startup, similar to the folllowing: `minetest.find_nodes_in_area() failed to find 218 nodes that were placed. Downgrading to Minetest 5.4.1 might fix this.`

cora merged commit 93a0879b40 into master

2021-12-20 23:43:01 +01:00

cora referenced this issue from a commit

2021-12-20 23:43:02 +01:00

Merge pull request 'MISC/mcl_selftests: Do not crash if minetest.find_nodes_in_area() lies' (#214) from fix-selftest-crash into master

cora deleted branch fix-selftest-crash

2021-12-20 23:43:04 +01:00

cora referenced this pull request

2021-12-20 23:48:30 +01:00

Mineclonia not starting in Minetest 5.5-dev due to faulty find_nodes_in_area() bugfix in Minetest #183

This repo is archived. You cannot comment on pull requests.

No reviewers