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

[24.2] Fix timeout handling for planemo / galaxy-tool-util #19384

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Jan 8, 2025

Planemo calls verify_tool with a maxseconds value, and verify_tool sets tool_test_dict.setdefault("maxseconds", maxseconds), which fails, since we are serializing and already setting the default maxseconds while parsing the test dict.

Fixes the IUC tool test timeouts not working when testing against 24.2. Broken in
9695ccc#diff-06835be1d1943ebce8373e2cd1f8e86c7b7813b720398850c978b734dd29e2daR1390 (maxseconds was previously not serialized at all in ToolTestDescription.to_dict)

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 25.0 milestone Jan 8, 2025
@mvdbeek mvdbeek requested a review from a team January 8, 2025 10:38
Planemo calls `verify_tool` with a maxseconds value,
and then sets `tool_test_dict.setdefault("maxseconds", maxseconds)`,
which fails, since we are serializing and already setting the default
maxseconds while parsing the test dict.

Fixes the IUC tool test timeouts not working when testing against 24.2.
Broken in
galaxyproject@9695ccc#diff-06835be1d1943ebce8373e2cd1f8e86c7b7813b720398850c978b734dd29e2daR1390
(maxseconds was prviously not serialized at all in
`ToolTestDescription.to_dict`)
@mvdbeek mvdbeek force-pushed the fix_maxseconds_parsing branch from 9f54384 to 05ede4a Compare January 8, 2025 11:17
@mvdbeek
Copy link
Member Author

mvdbeek commented Jan 8, 2025

Hmm, this does fix it for my local planemo test setup

diff --git a/tests/test_cmd_test.py b/tests/test_cmd_test.py
index 7d0475f33f..d034f34b44 100644
--- a/tests/test_cmd_test.py
+++ b/tests/test_cmd_test.py
@@ -114,7 +114,7 @@ class CmdTestTestCase(CliTestCase):
         """Test if --test_timeout parameter is working."""
         with self._isolate(), NamedTemporaryFile(prefix="timeout_test_json") as json_out:
             test_artifact = os.path.join(TEST_TOOLS_DIR, "timeout.xml")
-            test_command = self._test_command("--test_output_json", json_out.name)
+            test_command = self._test_command("--test_output_json", json_out.name, "--galaxy_root", "/Users/mvandenb/src/galaxy")
             test_command = self.append_profile_argument_if_needed(test_command)
             test_command += [
                 "--test_timeout",

but it seems to not have worked for https://github.com/galaxyproject/tools-iuc/actions/runs/12668894193/job/35305453205 where a single test has been running for more than an hour already.

@mvdbeek
Copy link
Member Author

mvdbeek commented Jan 8, 2025

Looks like it did eventually time out. Maybe we need to combine this with a limits section in the managed instances' job config and/or kill the job in the client

@mvdbeek mvdbeek merged commit e251c07 into galaxyproject:release_24.2 Jan 8, 2025
42 of 52 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.

2 participants