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

Rain test fails randomly #359

Closed
bmwiedemann opened this issue Sep 12, 2022 · 10 comments · Fixed by #365
Closed

Rain test fails randomly #359

bmwiedemann opened this issue Sep 12, 2022 · 10 comments · Fixed by #365
Assignees

Comments

@bmwiedemann
Copy link
Contributor

bmwiedemann commented Sep 12, 2022

Describe the bug
Rain test fails after 2038-01-19

To Reproduce

on openSUSE, I build the package with

osc co openSUSE:Factory/python-asciimatics
osc build --vm-type=kvm --noservice --clean --build-opt=--vm-custom-opt="-rtc base=2038-01-20T00:00:00" --alternative-project=home:bmwiedemann:reproducible openSUSE_Tumbleweed

Expected behavior

tests should continue to pass in future

Screenshots

 =================================== FAILURES ===================================
 ___________________________ TestParticles.test_rain ____________________________
 
 self = <tests.test_particles.TestParticles testMethod=test_rain>
 
     def test_rain(self):
         """     
         Test that Rain works as expected.
         """     
         screen = MagicMock(spec=Screen, colours=8)
         canvas = Canvas(screen, 10, 40, 0, 0)
         effect = Rain(canvas, 200)
 >       self.check_effect(canvas,
                           effect,
                           lambda value: self.assertIn(chr(value[0]), ' `\\v'))

 tests/test_particles.py:98:
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 tests/test_particles.py:43: in check_effect
     self.assertTrue(changed, "failed at step %d %s" % (i, view))
 E   AssertionError: False is not true : failed at step 0

==== 1 failed, 149 passed, 41 skipped, 1 deselected, 966 warnings in 3.67s =====

System details (please complete the following information):

  • OS and version: openSUSE-Tumbleweed-20220906
  • Python version: 3.10
  • Python distribution: cpython
  • Asciimatics version 1.14.0

Additional context
See also https://en.wikipedia.org/wiki/Year_2038_problem

As part of my work on reproducible builds for openSUSE, I check that software still gives identical build results in the future.
The usual offset is +16 years, because that is how long I expect some software will be used in some places.
This showed up failing tests in our package build.
See https://reproducible-builds.org/ for why this matters.

@peterbrittain
Copy link
Owner

Interesting... I completely agree that future stability is important, but will struggle to test this as I don't have an openSUSE build env.

First thing to check here is how reproducible this failure is... These tests are for deliberately randomized effects and so sometimes hit failures due to the random seed. If so, I can tweak the warm up for this test to let it run a little before checking output.

Does this always fail like this, or was it a one off?

@peterbrittain peterbrittain self-assigned this Sep 12, 2022
@bmwiedemann
Copy link
Contributor Author

Hmm. I cannot reproduce it anymore, so it probably really is more of a random failure, independent of the date.
Maybe use a fixed random seed then or let the test retry on failure?

@peterbrittain
Copy link
Owner

Yeah - seeding the RNG is a good idea. I'll push something to master.

@stale
Copy link

stale bot commented Nov 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 12, 2022
@peterbrittain
Copy link
Owner

Naughty stalebot! I'll fix this...

@stale
Copy link

stale bot commented Jan 16, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 16, 2023
@bmwiedemann bmwiedemann changed the title Rain fails after 2038-01-19 Rain test fails randomly Jan 16, 2023
@stale stale bot removed the wontfix label Jan 16, 2023
@bmwiedemann
Copy link
Contributor Author

I added random.seed(42) in various places but still get random failures in TestParticles.test_drop_screen and test_rain, unless I do it there:

--- asciimatics-1.14.0.orig/asciimatics/effects.py
+++ asciimatics-1.14.0/asciimatics/effects.py
@@ -12,7 +12,7 @@ from builtins import object
 from builtins import range
 from future.utils import with_metaclass
 from abc import ABCMeta, abstractmethod, abstractproperty
-from random import randint, random, choice
+from random import randint, random, choice, seed
 from math import sin, cos, pi
 from asciimatics.paths import DynamicPath
 from asciimatics.screen import Screen
@@ -54,6 +54,7 @@ class Effect(with_metaclass(ABCMeta, obj
         :param stop_frame: Stop index for the effect.
         :param delete_count: Number of frames before this effect is deleted.
         """
+        seed(42)
         self._screen = screen
         self._start_frame = start_frame
         self._stop_frame = stop_frame

@peterbrittain
Copy link
Owner

peterbrittain commented Jan 16, 2023

Interesting... Given that the random numbers are deterministic once the seed is set, the only reason it wouldn't follow the same path is because of some systemic difference. Are you running the test with multiple threads, or something like that?

@bmwiedemann
Copy link
Contributor Author

My guess is that randomness is used before tests run random.seed(42) e.g. in

self._maybe_reseed(True)

Which is why it became deterministic when adding it early in asciimatics/effects.py

@peterbrittain
Copy link
Owner

That doesn't quite make sense. Those should only get invoked once the class is constructed (i.e. in the test). I'd also like to avoid seeding inside the real code...

Did you try using the setUp() method of the unittest class to set the seed? Something like this:

class TestParticles(unittest.TestCase):
    def setUp(self):
        seed(42)

bmwiedemann added a commit to bmwiedemann/asciimatics that referenced this issue Jan 17, 2023
peterbrittain pushed a commit that referenced this issue Jan 17, 2023
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 a pull request may close this issue.

2 participants