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

More Test cleanup #534

Merged
merged 6 commits into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@e1000
Copy link

commented Sep 6, 2018

Some more test cleanups; it's an infinite job, but I want to believe ( that we might close #496 this year :) )

But this time, when I run all tests, I get :

Traceback (most recent call last):
  File "/usr/lib/python3.5/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.5/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "test/__main__.py", line 131, in <module>
    run_and_exit(*args, **kwds)
  File "/home/eea/src/pygame/test/test_utils/run_tests.py", line 339, in run_and_exit
    total, fails = run(*args, **kwargs)
  File "/home/eea/src/pygame/test/test_utils/run_tests.py", line 299, in run
    assert total == untrusty_total
AssertionError

which I don't understand. I checked the commit where I introduced it (917f364), but I still do not understand ...

Other thing, many tests depend on pygame.HAVE_NEWBUF, but I could find no documentation on this variable, where and when it's introduced, and if it can be false ...

self.assertEqual(d['data'], obj.data)
finally:
d = None
iface = proxy.__array_interface__

This comment has been minimized.

Copy link
@illume

illume Sep 7, 2018

Member

I wonder why you moved this out of a try finally block?

Probably the reason it assigns that to None is so that the destructor is called immediately.

This comment has been minimized.

Copy link
@e1000

e1000 Sep 7, 2018

Author

ok, I just wondered about all these del sth, I might have been going too far...
But why should the destructor be called immediately? After all, the tests pass without...

This comment has been minimized.

Copy link
@illume

illume Oct 9, 2018

Member

I guess this will cause issues on pypy, which doesn't call destructors when something leaves scope.

self.assertEqual(imp.strides, exp.strides)
self.assertTrue(imp.suboffsets is None)
finally:
imp = None

This comment has been minimized.

Copy link
@illume

illume Sep 7, 2018

Member

Similar to previous comment, this imp = None part is probably needed (I'm guessing).

This comment has been minimized.

Copy link
@e1000

e1000 Sep 7, 2018

Author

Same question as above: why would it be needed? Do you think, it could be related to the AssertionError crash above ? I'll try it out..

This comment has been minimized.

Copy link
@e1000

e1000 Sep 7, 2018

Author

no, my remark was non sens, it can not be related since the crash happens as of commit (917f364) where as the del are removed on my last commit :(

@illume

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Looks like PG_ENABLE_NEWBUF is defined in src_c/pgcompat.h and is enabled by default (unless pypy).

I'm pretty sure that pypy now also supports the new buffer protocol. Because they have done a lot of improvements to their C extension support in the last years. I changed all that pgcompat.h to #define PG_ENABLE_NEWBUF 1 and it worked in pypy too (I ran examples.arraydemo.py at least).

So, it seems we can get rid of all that not newbuffer stuff.

@e1000

This comment has been minimized.

Copy link
Author

commented Sep 7, 2018

I don't know about PG_ENABLE_NEWBUF and also PG_ENABLE_OLDBUF , but I can do the cleanup if you are sure !

As about the above crash, do you have any hint or documentation ? I came across before, but I don't remember what solved the problem. A more explaining Error message would be nice, I think...

e1000
test machinery : adapt to skip & expectedFailure decorators
s means skipping...
x means expectedFailure...
@e1000

This comment has been minimized.

Copy link
Author

commented Sep 7, 2018

got it for the crash ! it was the test machinery not accepting letters 's' (skipping) and 'x' (expected failure);

(at least I hope so, because it seemed to behave randomly, but the mistake was problably in front of the computer ...)

@e1000

This comment has been minimized.

Copy link
Author

commented Sep 7, 2018

should I reintroduce the try/finally for immediate destructor calling ? Because my guess was more that it was one of many unnecessary test code artefacts ...

@illume

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Thanks 🎉

ps. Again... sorry for the delay.

@illume illume merged commit 4bea7ab into pygame:master Oct 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illume illume referenced this pull request Oct 16, 2018

Closed

1.9.5 release notes. #561

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.