Surface.test_get_abs_parent and Surface.test_get_abs_offset tests, and fix.

[lkito]

Wrote unit tests as per issues #1797 and #1798

Did I need to write the display.quit case as in #1898 ?
Because if so, at that point, wouldn't it make sense to have a different unit test just for display.quit case and just test there? The discussions of that bug would also be in a single (new)issue and it would be more convenient.

[lkito]

I just saw that the display.quit bug has been resolved, but my point still stands: I think it makes sense to have a different unit test for that. If i'm wrong, I'll add to this and the get_bitsize unit tests as you'd like.

[MyreMylar]

I think we probably need to test it for every (32 of them) functions that include the assert as it is right now - mainly because it is written into the code 32 separate times and at any time somebody could remove or accidentally break one of those code blocks and break that particular function while leaving the others intact.

This sort of thing is why the unit tests have sat undone for a long time I expect.

[lkito]

Hey, sooo, the problem got fixed with get_bitsize. I just pushed the updated test for that, but get_abs_parent() and get_abs_offset() still don't raise an error.

with self.assertRaises(pygame.error):
            surface = pygame.display.set_mode()
            print(surface.get_abs_parent())
            pygame.display.quit()
            print(surface.get_abs_parent())

prints

<Surface(1366x768x32 SW)>
<Surface(Dead Display)>

os.environ['SDL_VIDEODRIVER'] = 'dummy' still doesn't change anything.

[lkito]

Are these methods supposed to have the same checks?
If so, I can add these to those 2 methods. (I tried adding this and it throws the error)

SDL_Surface *surf = pgSurface_AsSurface(self);
if (!surf)
    return RAISE(pgExc_SDLError, "display Surface quit");

[MyreMylar]

Huh, looks like that's because these two are some of the few functions that don't raise this assert in surface.c.

EDIT: I see you spotted that as I was typing...

I feel like they should have this assert, since pretty much everything else in surface.c does and the functions don't actually work after the display has been quit.

EDIT 2: I count 49 documented functions in the surface module and 37 instances of this assert being raised. So I guess it is potentially missing from 12 of these functions. Though perhaps some legitimately do still work after the display module has been quit...

[lkito]

I did the thing with some other related methods and their unit tests, too. Let me know if I went overboard, or the checks could be done more efficiently.

[lkito]

I'm not sure, but did the tests fail because I'm supposed to declare variables first and then have expressions? If so, is there anything I can use to check before pushing? I don't remember seeing anything like that in contribution guide

[MyreMylar]

I'm not sure, but did the tests fail because I'm supposed to declare variables first and then have expressions? If so, is there anything I can use to check before pushing? I don't remember seeing anything like that in contribution guide

That is a weird python 2 only thing, that probably should be written up somewhere in a tips section for writing C Python code. I recall discovering it in the same way.

[lkito]

======================================================================
FAIL: test_delay (pygame.tests.time_test.TimeModuleTest)
Tests time.delay() function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python35-x64\lib\site-packages\pygame\tests\time_test.py", line 144, in test_delay
    self._wait_delay_check(pygame.time.delay, millis, iterations, delta)
  File "C:\Python35-x64\lib\site-packages\pygame\tests\time_test.py", line 210, in _wait_delay_check
    self.assertAlmostEqual(wait_time, millis, delta=delta)
AssertionError: 88 != 50 within 5 delta

I just searched for fails and errors and this is the only thing I could find.
It says there are syntax errors, but there is none I could find. No idea what's going on, this makes no sense

[MyreMylar]

======================================================================
FAIL: test_delay (pygame.tests.time_test.TimeModuleTest)
Tests time.delay() function.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Python35-x64\lib\site-packages\pygame\tests\time_test.py", line 144, in test_delay
    self._wait_delay_check(pygame.time.delay, millis, iterations, delta)
  File "C:\Python35-x64\lib\site-packages\pygame\tests\time_test.py", line 210, in _wait_delay_check
    self.assertAlmostEqual(wait_time, millis, delta=delta)
AssertionError: 88 != 50 within 5 delta

I just searched for fails and errors and this is the only thing I could find.
It says there are syntax errors, but there is none I could find. No idea what's going on, this makes no sense

Yes that is another new unit test added this week that has variable accuracy on Continuous Integration which it seems we are still trying to nail down.

[lkito]

I guess that means that I'll have to wait for that one.
I think I fixed the syntax error. Still doesn't make sense why it says that there were syntax errors, though.

[MyreMylar]

I guess that means that I'll have to wait for that one.
I think I fixed the syntax error. Still doesn't make sense why it says that there were syntax errors, though.

Where are you seeing syntax errors reported? Is it on one of the CI builds you could link me to?

[lkito]

Jesus christ, I think I somehow mistook my own commit message for the build error.
Nevermind, there are no syntax errors. It's just that test then

[MyreMylar]

CI for this test should be fixed by this PR:
#1914

[illume]

👍

[illume]

Thanks for this :)