search

LinuxCNC / linuxcnc

LinuxCNC controls CNC machines. It can drive milling machines, lathes, 3d printers, laser cutters, plasma cutters, robot arms, hexapods, and more.
http://linuxcnc.org/
GNU General Public License v2.0
1.77k stars 1.14k forks source link

Skip test linuxcncrsh until it become more stable. #2824

Closed petterreinholdtsen closed 8 months ago

petterreinholdtsen commented 8 months ago

As can be seen in several test failures on github and discussed in https://github.com/LinuxCNC/linuxcnc/pull/2739, the linuxcncrsh test is unstable. It is believed to be caused by a race condition. Until the race condition is fixed, I believe it is best to skip the test.

heeplr commented 8 months ago

Rather than disable the test completely, it might be better to just comment out the line triggering the race condition.

Also for the record: It's not linuxcncrsh that is unstable. The race condition existed for a long time but just didn't ever get tested for before I added more tests.

petterreinholdtsen commented 8 months ago

[heeplr]

Rather than disable the test completely, it might be better to just comment out the line triggering the race condition.

Perhaps, if it can be done as quickly as this patch. I assume your hunt for a proper fix will soon be successful, and that the test will not stay disabled for long. :)

Also for the record: It's not linuxcncrsh that is unstable. The race condition existed for a long time but just didn't ever get tested for before I added more tests.

Good to know, and good work. :) More testing is good. :) -- Happy hacking Petter Reinholdtsen

heeplr commented 8 months ago

I assume your hunt for a proper fix will soon be successful

I wouldn't expect too much since my linuxcnc-todo list is quite long and this issue could be fixed much quicker by someone familiar with milltask innards. Also I won't spend a single minute anymore with "malformatting" my PRs so the process mentioned in #2760 needs to be completed first.

More testing is good. :)

I absolutely agree. But only if the bugs bubbling up by more testing are actually fixed ;) Generally I'd suggest that failing tests shouldn't block the test builds, since test builds can contain errors by definition.

andypugh commented 8 months ago

This test is still failing sometimes, so whilst agreeing with what you say, I will merge this.

heeplr commented 6 months ago

Still would be wiser to just disable the failing commands inside the test instead of disabling the complete test.

(I don't really care since I still think there should be -dev/-test builds that keep track of failing tests but without blocking development of other parts of the code. But I guess the underlying bug won't be fixed anytime soon and no one is using linuxcncrsh anyway, so I'm good.)