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

Run CI Daily #1862

Open
wants to merge 4 commits into
base: 0.10
Choose a base branch
from

Conversation

DaAwesomeP
Copy link
Member

This pull causes the CI to run everyday at 12am UTC. This should hopefully help catch issues before they end up failing in unrelated pull requests. This should definitely help catch the frequent new codespell matches.

@DaAwesomeP
Copy link
Member Author

There seems to be a race condition with python/ola/ClientWrapperTest.sh. It failed on bullseye last run and then this run in sid.

Fitting that this is discovered in this pull request of all pull requests.

@DaAwesomeP
Copy link
Member Author

The error:

FAIL: python/ola/ClientWrapperTest.sh
=====================================

...F/usr/lib/python3.11/unittest/case.py:622: ResourceWarning: unclosed <socket.socket fd=4, family=1, type=1, proto=0>
  with outcome.testPartExecutor(self):
ResourceWarning: Enable tracemalloc to get the object allocation traceback
..
======================================================================
FAIL: testEventLoop (__main__.ClientWrapperTest.testEventLoop)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/ola/ola/./python/ola/ClientWrapperTest.py", line 209, in testEventLoop
    self.assertAlmostEqual(d_diff, datetime.timedelta(milliseconds=15),
AssertionError: datetime.timedelta(microseconds=17990) != datetime.timedelta(microseconds=15000) within datetime.timedelta(microseconds=750) delta (datetime.timedelta(microseconds=2990) difference)

----------------------------------------------------------------------
Ran 6 tests in 0.021s

FAILED (failures=1)
FAIL python/ola/ClientWrapperTest.sh (exit status: 1)

@peternewman
Copy link
Member

peternewman commented Jun 22, 2023

Yeah this has unfortunately been kicking around for a while, it's supposed to respond within a 750 uS window and its taken 2990.

microseconds=750) delta (datetime.timedelta(microseconds=2990

# Check when the callbacks were called. Allow 500 microseconds of drift.
# Called immediately
a_diff = results.a_called - self.start
self.assertAlmostEqual(a_diff, datetime.timedelta(milliseconds=0),
delta=datetime.timedelta(microseconds=750))
# Called in 5 milliseconds
b_diff = results.b_called - self.start
self.assertAlmostEqual(b_diff, datetime.timedelta(milliseconds=5),
delta=datetime.timedelta(microseconds=750))
# Called in 10 milliseconds
c_diff = results.c_called - self.start
self.assertAlmostEqual(c_diff, datetime.timedelta(milliseconds=10),
delta=datetime.timedelta(microseconds=750))
# Called in 15 milliseconds
d_diff = results.d_called - self.start
self.assertAlmostEqual(d_diff, datetime.timedelta(milliseconds=15),
delta=datetime.timedelta(microseconds=750))

Although now writing this I'm slightly suspicious that's its micro and I wonder/assumed it was supposed to be milli (without rechecking the code). Having checked micro makes sense, given we're asking for tens of milliseconds for the window. It does inherently need to be fairly tight unfortunately, due to the nature of the check it's running.

But either way its definitely unfortunately worse on our various VMs/images.

@peternewman peternewman added this to the 0.10.10 milestone Jun 22, 2023
Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the general sense, I wonder if we should stagger things a bit though, I guess it rather depends on the frequency of PRs coming in, but is daily an appropriate frequency for doing all the full builds? I wonder if they should be less often given only external package changes can break them.

I'm more onboard with doing the spelling CI regularly given as you say that will often get tripped up by external factors.

The other thing is unless these raise an issue or something, will anyone actually spot the CI issues?

@DaAwesomeP
Copy link
Member Author

I can see the general sense, I wonder if we should stagger things a bit though, I guess it rather depends on the frequency of PRs coming in, but is daily an appropriate frequency for doing all the full builds? I wonder if they should be less often given only external package changes can break them.

My thinking is that daily/nightly allows for us to find those external breaking factors before they make it into an unrelated PR. We could switch to weekly if you would prefer. We could run lint daily and builds weekly.

I'm more onboard with doing the spelling CI regularly given as you say that will often get tripped up by external factors.

I think running all of it on some frequency is nice because you find changes that you really aren't expecting (i.e. maybe Debian changes how Python libraries are installed or Autoconf behavior changes in a new version). Importantly, it's much nicer to find these changes one at a time and not many at once after a longer period of inactivity on the repo.

The other thing is unless these raise an issue or something, will anyone actually spot the CI issues?

IIRC repo watchers will get a notification that a run failed.

This was referenced Jul 11, 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 this pull request may close these issues.

2 participants