pcdshub / typhos

Automatic-yet-customizable Graphical User Interface Generation for Ophyd Devices
http://pcdshub.github.io/typhos
Other
16 stars 26 forks source link

CI/TST: Fix CI #607

Closed ZLLentz closed 4 months ago

ZLLentz commented 4 months ago

Description

This PR resolves the following CI issues:

The PR fixes these largely by adding a no_gc pytest mark that skips the gc.collect() call that causes some of the tests to have a segmentation fault and a no_cleanup_check mark that skips the post-test cleanup assert.

In short: skip these in the cases where they don't work.

The other changes here are:

Motivation and Context

Need to be able to rely on the results from the CI test suite for proper development flow Needed to make sure the issues weren't "real" (affecting users) Need to move on and work on other things

How Has This Been Tested?

Reproduced the issue offline, understood when it happens, adjusted tests

Where Has This Been Documented?

Here only

Pre-merge checklist

ZLLentz commented 4 months ago

Next week:

tangkong commented 4 months ago

Just testing my understanding here: Is the garbage collection you're turning off the manual garbage collection we placed into the typhos test suite right? Normal pytest / python garbage collection remains?

Is there any rhyme or reason to avoiding garbage collection? It looks like everything with motor widgets (things involving a separate thread that isn't resolving before gc?)

ZLLentz commented 4 months ago

Is the garbage collection you're turning off the manual garbage collection we placed into the typhos test suite right? Normal pytest / python garbage collection remains?

Yes, this is just the manual gc.collect() call that I'm disabling here

Is there any rhyme or reason to avoiding garbage collection?

I noticed that any time two tests with positioner widgets were run in the same test suite execution, the second one would segfault on the gc.collect() call. I haven't thought very hard about why this is the case. This is true no matter what you do with the positioner widget, so long as the uic load step is run.

I decided that, in general, it was better to collect more garbage to avoid cross-test contamination, but since it's failing here I want to skip it and move on. Perhaps with more time we could figure out why the test segfaults on gc.collect() in more detail.

ZLLentz commented 4 months ago

test failure is api.MAMBA_DOWNLOAD_FAILFAST marking as ready for review, asking for a review, and then re-running the failure

ZLLentz commented 4 months ago

Well, it didn't give me the option to only re-run the failure, so we're rerunning everything now!

ZLLentz commented 4 months ago

Ok great! Passing! Hopefully once reviewed and merged and I can finalize some of the other PRs for review (since the test suite will be useful again)

ZLLentz commented 4 months ago

We should record somewhere that using the positioner / motor fixtures require this no-gc mark. Since I'm sure to forget that offscreen-mode is necessary to observe these errors

I'm going to look at the repo and find the best spots to put this info into the development docs

Agreed on the squash, I had some interesting commit iterations here

ZLLentz commented 4 months ago

I decided to write some prose about the test suite, including this info and some other important things.

ZLLentz commented 4 months ago

ok I'm actually done now and not going to add another tiny commit if the docs build comes out rendering normally and the text seems ok I think this is done

ZLLentz commented 4 months ago

Seems to render OK I might have angered the CI gods by causing so many test suite runs though image

tangkong commented 4 months ago

At least we know we're not eating slaclab's resources now 😆

ZLLentz commented 4 months ago

I think this is ready to be squashed now (unless I have another typo/inaccuracy/missing info to fix in the docs)