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

use conda-index, disable questionable test #693

Merged
merged 4 commits into from
Apr 26, 2024
Merged

Conversation

DerThorsten
Copy link
Collaborator

No description provided.

@DerThorsten DerThorsten changed the title Verbose use conda-index, disable questionable test Apr 25, 2024
@DerThorsten
Copy link
Collaborator Author

DerThorsten commented Apr 25, 2024

This PR remove some tests with a questionable logic.

These tests more or less look like

@pytest.mark.timeout(1)
def test_it(
# .. some fixtures here
)
  p = Process(target=cli.app, args=(["start", config_dir, "--port", "8001"],))
  with Interrupt():
        p.start()
  p.join()
  assert p.exitcode == 0

The implementation of the Interrupt context is:

class Interrupt:
    # Interrupt child when SIGALRM is received.
    # Useful to kill the server when it is correctly launched, using a timeout.
    def _handle_interrupt(self, signum, frame):
        for p in active_children():
            p.terminate()
            p.join()

    def __enter__(self):
        signal.signal(signal.SIGALRM, self._handle_interrupt)

    def __exit__(self, exc_type, exc_val, exc_tb):
        pass

So first thing to notice is that the Interrupt context manager is not meaningful since the exit method does nothing.
A call to function like install_children_interruptor would do the same

def install_children_interruptor():
    def _handle_interrupt(self, signum, frame):
        for p in active_children():
            p.terminate()
            p.join()
    signal.signal(signal.SIGALRM, _handle_interrupt)

install_children_interruptor()

But ignoring this, I think the logic of the test might have been:

  • the test times out
  • This may fires SIGALRM since @pytest.mark.timeout(1) is used
  • This should trigger the installed signal handler
  • This should terminate the thread
  • We finally test if the process (which runs the server) was exited gracefully with an exit-code of 0

But long story short, these tests time out in the CI.
Given their somewhat complicated logic/flow, and how little they test, I suggest we remove them.

@DerThorsten DerThorsten requested a review from wolfv April 25, 2024 07:39
@DerThorsten DerThorsten marked this pull request as ready for review April 25, 2024 07:56
@wolfv wolfv merged commit 56ab2cf into mamba-org:main Apr 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants