Add tests for submitter and improve coverage ignoring pass/NotImplementedError #2604

kinow · 2025-09-17T09:28:27Z

As part of #2428 there were some changes that are not entirely pertinent to that issue/PR, so I've moved them to this one. It includes ignores for coverage, tests (and making submitter easier to test), and linter errors + types.

Check List

I have read CONTRIBUTING.md.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to pyproject.toml.
Tests are included (or explain why tests are not needed).
Changelog entry included in CHANGELOG.md if this is a change that can affect users.
Documentation updated.
If this is a bug fix, PR should include a link to the issue (e.g. Closes #1234).

codecov-commenter · 2025-09-17T09:40:33Z

Codecov Report

❌ Patch coverage is 94.11765% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.60%. Comparing base (a27a9c0) to head (205055d).

Files with missing lines	Patch %	Lines
autosubmit/platforms/paramiko_platform.py	85.07%	6 Missing and 4 partials ⚠️
autosubmit/platforms/wrappers/wrapper_factory.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2604      +/-   ##
==========================================
+ Coverage   65.86%   67.60%   +1.74%     
==========================================
  Files          84       84              
  Lines       19697    19564     -133     
  Branches     3815     3809       -6     
==========================================
+ Hits        12974    13227     +253     
+ Misses       5771     5419     -352     
+ Partials      952      918      -34

Flag	Coverage Δ
fast-tests	`67.60% <94.11%> (+1.74%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

kinow · 2025-09-17T11:10:12Z

Still need tests for restore_connection and a few parts of ParamikoPlatform. I guess I could try 100% on ParamikoPlatform since I'm already writing tests for this, and the class is pretty important. Then I'll deal with rebase/conflicts in #2428 , and that PR should have a lot less changes 🤞 Will ask @VindeeR to review it tomorrow or Friday if I've finished it (this PR, the retries/connection will probably go to @dbeltrankyl to review that one later)

kinow · 2025-09-17T11:10:58Z

Link to Codecov issue about not respecting ignored lines -- codecov/codecov-action#1326

kinow · 2025-09-18T12:01:52Z

@manuel-g-castro updated this one with more tests for Paramiko. Let's see if test-slurm passes 🤞

kinow · 2025-09-22T07:48:09Z

Rebased, triggering a new build as I think now it will succeed with the new slurm docker container 🤞

kinow · 2025-09-22T13:43:52Z

Fixed the slurm errors, rebased, and added more tests today. Now slowly going through missing lines in ParamikoPlatform to have enough tests for 4.1.16 💪

kinow · 2025-09-23T11:06:56Z

To check later why x11 didn't work in my local tests: #1321 (also check if wayland). Thanks @dbeltrankyl for the link & tip.

kinow · 2025-09-29T12:03:10Z

Uploaded container to https://hub.docker.com/r/autosubmit/linuxserverio-ssh-2fa-x11. Now we have a small box to test SSH + 2FA + X11. Let's see if we can finally test this code without mocks...

kinow · 2025-10-22T07:02:59Z

@VindeeR , I've covered as much of the code in paramiko_plataform.py as I could. There are some lines in def connect that are not covered here, but the PR from where this came off (#2428) has some tests for that, so the coverage in that part might be a bit better after that one is merged.

Once GH Actions build passes, this should be ready for review. There is a bunch of changes, but I think if you exclude the ones for coverage and linting, and the tests, that should leave you with just some parts of the code that were modified.

VindeeR

I learned a lot during this review, so many things I never knew I could implement the way you did, thanks for it 🙌

I've done a few comments but I have nothing major to point out, I believe everything to be working just fine, just asked a few things to understand better.

autosubmit/platforms/paramiko_platform.py

VindeeR · 2025-10-22T08:23:38Z

autosubmit/platforms/paramiko_submitter.py

+
+def _get_serial_platforms(platforms_used: set[str], platforms_data: dict) -> dict[str, list]:
+    """Traverse used platforms and look for serial platforms."""
+    serial_platforms = defaultdict(list)


The more you know, this kind of resource sounds really useful! Thanks ❤️

It is! I also learn a lot (Python/shell/AS/pytest/etc.) reviewing code :slig

...ssh/linuxserverio-ssh-with-2fa-x11/rootfs/etc/s6-overlay/s6-rc.d/init-openssh-mfa-config/run

VindeeR · 2025-10-22T09:51:17Z

test/integration/test_paramiko_platform.py

+    # FIXME: I thought this was supposed to be equal to the number of hetsize,
+    #        and at some point during debugging it it; but then there is a call
+    #        to other functions that reset it to just one hetjob. Looks like it
+    #        might be better to create the job from the dictionary configuration,
+    #        instead of trying to create the object here.


Sorry to nitpick but the message is a bit confusing at the beginning

Will rewrite it! 💪 thanks for proof-reading it!

Clarified a bit, or tried at least. Here's the updated text (incoming in some minutes in a new push).

# FIXME: I thought this was supposed to be equal to the number of hetsize # components (2, set above), and at some point during debugging it is; # but then there is a call to other functions somewhere that reset it # to just one hetjob. Looks like it might be better to create the job # from the dictionary configuration, instead of trying to create the # object here (i.e. an integration test that loads everything from # YAML configuration).

VindeeR · 2025-10-22T10:01:29Z

test/unit/platforms/test_paramiko_submitter.py

+    submitter.load_platforms(as_conf, None, None)
+    assert len(submitter.platforms) == 2
+
+    assert 'ecaccess' not in submitter.platforms


I'm a bit confused by this test, you're loading the platform from the config into the submitter plus the Paramiko, but the platform ecaccess is not within the submitter even though there are 2 platform there 😵‍💫

I've updated the python documentation of that function. You should be able to review it in an updated commit that I'm pushing in 2 minutes, but here's the whole new text.

def test_ecplatform_fails_without_crashing(autosubmit_config): """Test that ecaccess platform is ignored when it does not have a version. Not sure if it should fail without crashing the execution, but... the current code silently ignores the platform. Note that the configuration contains the ecaccess platform. And a job is using it. However, there is no version in the ecaccess platform. It needs a version like PBS or Slurm platform, which will be used with/in conjunction (my understanding of the platform). In the end, the test verifies that there are two platforms loaded, which are always present, even though it's a single platform, the local, aliased as LOCAL (i.e. a dictionary with two entries to the same object, the local platform object). This code is quite confusing and error-prone, but having this test should be a good starting point. """

I think this should be clearer. Thanks for reviewing that! I would probably have forgotten and struggled a bit to remember what I meant with that comment 🤣

VindeeR · 2025-10-22T11:37:19Z

I think once you do the update of the branch you can merge it with no problems, thanks for the hard work 🙌
Improving the coverage a lot on this one

kinow · 2025-10-22T12:55:02Z

I think once you do the update of the branch you can merge it with no problems, thanks for the hard work 🙌 Improving the coverage a lot on this one

Thanks for the thorough review, @VindeeR ! Updated the comments replying/marking as resolved the ones I think were OK, and leaving the other ones which I updated something without resolving, so that you can have another look at them 🙂

It should be ready to review again once GH Actions pass. Cheers.

kinow · 2025-10-22T13:15:10Z

Rebased.

VindeeR

Changes look great thanks for having a look at them 👍

kinow · 2025-10-22T13:29:01Z

We can now both rebase our branches. I will rebase #2428 tomorrow/Friday. Let me know once your #2577 and I will review that one 👍 Thanks @VindeeR !

kinow changed the title ~~Add tests submitter and improve coverage ignoring pass/NotImplementedError~~ Add tests for submitter and improve coverage ignoring pass/NotImplementedError Sep 17, 2025

kinow force-pushed the add-tests-submitter-coverage branch from 67bae16 to 9f71998 Compare September 17, 2025 09:34

kinow force-pushed the add-tests-submitter-coverage branch 2 times, most recently from f370634 to 57169d1 Compare September 17, 2025 10:00

kinow self-assigned this Sep 17, 2025

kinow added this to the 4.1.16 milestone Sep 17, 2025

kinow force-pushed the add-tests-submitter-coverage branch 3 times, most recently from 550bd93 to aa19601 Compare September 17, 2025 11:07

kinow force-pushed the add-tests-submitter-coverage branch from aa19601 to 81addb4 Compare September 18, 2025 12:01

kinow force-pushed the add-tests-submitter-coverage branch from 81addb4 to 9585f49 Compare September 22, 2025 07:44

kinow force-pushed the add-tests-submitter-coverage branch 4 times, most recently from 6989b8a to 540714e Compare September 22, 2025 13:43

kinow force-pushed the add-tests-submitter-coverage branch from 540714e to 8dcc8ee Compare September 22, 2025 13:45

kinow mentioned this pull request Sep 29, 2025

Remove nontransitive from functions, add mypy type hints, fix linter issues #2577

Merged

8 tasks

kinow force-pushed the add-tests-submitter-coverage branch 4 times, most recently from e710163 to 8d55153 Compare October 1, 2025 14:42

kinow mentioned this pull request Oct 1, 2025

Handle retries for SSH connection (2 retries) and command execution (3 retries) #2428

Open

12 tasks

kinow force-pushed the add-tests-submitter-coverage branch 15 times, most recently from 9228769 to fc8e424 Compare October 22, 2025 07:00

kinow marked this pull request as ready for review October 22, 2025 07:00

kinow requested a review from VindeeR October 22, 2025 07:01

kinow mentioned this pull request Oct 22, 2025

CSCDESTINCLIMADT-794: Fix 'list' object has no attribute 'status' #2474

Draft

8 tasks

VindeeR approved these changes Oct 22, 2025

View reviewed changes

kinow force-pushed the add-tests-submitter-coverage branch from fc8e424 to 0911de6 Compare October 22, 2025 12:54

Add tests for Submitter, simplify code, add types and tests.

205055d

kinow force-pushed the add-tests-submitter-coverage branch from 0911de6 to 205055d Compare October 22, 2025 13:15

VindeeR approved these changes Oct 22, 2025

View reviewed changes

kinow merged commit 7fa1d7c into master Oct 22, 2025
21 of 22 checks passed

kinow deleted the add-tests-submitter-coverage branch October 22, 2025 13:27

Add tests for submitter and improve coverage ignoring pass/NotImplementedError #2604

Add tests for submitter and improve coverage ignoring pass/NotImplementedError #2604

Uh oh!

Conversation

kinow commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kinow commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinow commented Sep 17, 2025

Uh oh!

kinow commented Sep 18, 2025

Uh oh!

kinow commented Sep 22, 2025

Uh oh!

kinow commented Sep 22, 2025

Uh oh!

kinow commented Sep 23, 2025

Uh oh!

kinow commented Sep 29, 2025

Uh oh!

kinow commented Oct 22, 2025

Uh oh!

VindeeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

VindeeR Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

kinow Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

VindeeR Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinow Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

kinow Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

VindeeR Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

kinow Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VindeeR commented Oct 22, 2025

Uh oh!

kinow commented Oct 22, 2025

Uh oh!

kinow commented Oct 22, 2025

Uh oh!

VindeeR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kinow commented Oct 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinow commented Sep 17, 2025 •

edited

Loading

codecov-commenter commented Sep 17, 2025 •

edited

Loading

kinow commented Sep 17, 2025 •

edited

Loading

VindeeR Oct 22, 2025 •

edited

Loading

kinow Oct 22, 2025 •

edited

Loading