Repository navigation
Conversation
|
After this pull request, the command-line parsing part of the code, in There's still the bug that the |
1663271 to
5a37871
Compare
670c8ac to
77a2008
Compare
kinow
changed the title
kinow
changed the title
d032274 to
1a79f17
Compare
| basic_config.MAIL_FROM = 'notifier@localhost' | ||
| basic_config.SMTP_SERVER = f'127.0.0.1:{smtp_port}' | ||
| monkeypatch.setattr(BasicConfig, 'MAIL_FROM', 'notifier@localhost') | ||
| monkeypatch.setattr(BasicConfig, 'SMTP_SERVER', f'127.0.0.1:{smtp_port}') |
| 'FAILED': [] | ||
| } | ||
| yield platform | ||
| local_root_dir.cleanup() |
| basic_config.MAIL_FROM = 'notifier@localhost' | ||
| basic_config.SMTP_SERVER = f'127.0.0.1:{smtp_port}' | ||
| monkeypatch.setattr(BasicConfig, 'MAIL_FROM', 'notifier@localhost') | ||
| monkeypatch.setattr(BasicConfig, 'SMTP_SERVER', f'127.0.0.1:{smtp_port}') |
| config_file = tmp_path / "autosubmitrc" | ||
| config_file.write_text(config_content) | ||
| os.environ = {'AUTOSUBMIT_CONFIGURATION': str(config_file)} | ||
| monkeypatch.setenv("AUTOSUBMIT_CONFIGURATION", str(config_file)) |
There was a problem hiding this comment.
This is safer, and avoids bugs. Running this test with others that access the env var AUTOSUBMIT_CONFIGURATION could result in unexpected issues (and that are hard to explain/understand).
| f"base_{job_type.lower()}_{scheduler.lower()}.cmd")).read_text() | ||
| if not expected_data: | ||
| assert False, f"Could not find the expected data for {scheduler} and {job_type}" | ||
| pytest.fail(f"Could not find the expected data for {scheduler} and {job_type}") |
There was a problem hiding this comment.
Fixing a SonarQube warning; PyCharm doesn't see this warning/error, interesting. asserting False also works... but probably better to use pytest.fail. Fixed that somewhere else here...
| CONFIG_FILE_FOUND = False | ||
| DATABASE_BACKEND = "sqlite" | ||
| DATABASE_CONN_URL = "" | ||
| LOG_RECOVERY_TIMEOUT: int = 60 * 5 |
There was a problem hiding this comment.
This was not defined in __init__, but somewhere else, in a function. The constructor above is not needed either.
a27aded to
0da64a4
Compare
| # expids a lot. This is because we do not really have experiments, it's a "fake" layer, | ||
| # and instead we only create a configuration instance here, and return it. Thus, the | ||
| # new instance is always created and used here, and not the configuration registry. | ||
| config = AutosubmitConfig(expid=expid) |
There was a problem hiding this comment.
Just explaining why in the other conftest we have load_config, but not here.
f8a3401 to
3a0ebdd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3327 +/- ##
==========================================
- Coverage 85.28% 85.28% -0.01%
==========================================
Files 148 149 +1
Lines 22173 22171 -2
Branches 3934 3936 +2
==========================================
- Hits 18911 18908 -3
Misses 2501 2501
- Partials 761 762 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| self.ignore_file_path = False | ||
| self.expid = expid | ||
| self.basic_config = basic_config | ||
| self.basic_config.read() |
There was a problem hiding this comment.
This call is not needed.
The AutosubmitConfig class is created in @cli_function, in _cli_function.py. Before that, in _initialise.py, we make a call to BasicConfig.read().
That was done copying previous existing behaviour in autosubmit.py, but it was harder to see whether that was needed or not.
I've kept that call in _initialise.py, replaced the TODO asking if that was needed by a short comment explaining why we have that call there. And now when we get here, it' s on the assumption that BasicRead.read() was called (and other assumptions, like an existing working directory now, also system executables available, the required directories created, etc.).
| dedent("""\ | ||
| CONFIG: | ||
| SAFETYSLEEPTIME: 0 | ||
| SAFETYSLEEPTIME: 3 |
There was a problem hiding this comment.
Using 3 instead of 0 as I noticed Slurm was suffering being hammered with SSH send_command calls. This should make our tests less brittle on GitHub Actions, as it will give Slurm some more time to process things, before getting a new SSH or Slurm command.
|
|
||
| session_mocker.patch('time.sleep', side_effect=my_sleep) | ||
|
|
||
|
|
There was a problem hiding this comment.
Removed as I noticed that the SSH server was getting a lot of requests every second 😶🌫️
I replaced two sleep calls in the last few months. I will chase other sleep calls in our tests, and try to remove them too. Our safety sleep time in the tests has been changed to three seconds. With that, I hope the tests with containers may finish sooner.
|
I will try to add more tests to cover all |
|
But maybe you can do a review, @VindeeR. |
The main goal with this is to centralise the creation of AutosubmitConfig objects, and reduce the risk of re-loading the configuration accidentally.
bfb6860 to
7d68a8c
Compare
|
Ok. This is a rabbit hole. @VindeeR I had a look at the coverage, and it's not very hard to fix one or two of the items pending here. They are simple, but I thought it'd still be worth adding the tests. To do so, first I had to go around the AutosubmitConfig issues, especially type errors. So, I started fixing type errors, and adding docstrings. And the new commit got gigantic 😅 I've stashed it, and will push to another branch to fix mypy/docstrings and especially the type of parameters returned in AutosubmitConfig, so writing tests for those is a bit easier. I've rebased, and left two commits only. The one adding a registry, and the other removing the link between AutosubmitConfig & BasicConfig, and updating tests (and some docstring/mypy 😶🌫️ ). So from yesterday there shouldn't be any change, I believe, except for the rebase. |
|
For my future self: kinow/autosubmit@cc7d959 Your commit with the work-in-progress to improve coverage in autosubmit.config, and reduce mypy errors (remember to disable follow-imports). |
|
72 files changed, but 20 are source, 1 pytest.ini and the rest are the tests updated due to changes in AutosubmitConfig constructor and to use I think it should be OK to include this one in 4.2.0 as it doesn't change any user-facing feature, only how configuration objects are handled internally. Autosubmit is still loading the config twice, but it's because of the as_misc.yml mtime constantly changing (which is why the PR has "Related #..." and not "Closes #..."). |
1 participant
Related to #3326
This pull request creates a central configuration registry. This works to give commands an easy way to retrieve a configuration object that may already have been loaded elsewhere.
By centralising the config, we are now able to confirm nowhere else
basic_configoryaml_factoryare modified. And since those arguments use the same value, in the future we can remove from the constructor list of parameters, simplifying function signature (lots of tests using it, so leaving for later to reduce the change here).Added a second commit removing the indirect access to
BasicConfigviaAutosubmitConfigor other test classes. Now, the whole code accessesBasicConfigvia its import +BasicConfig, andAutosubmitConfig's only dependency onBasicConfigis a call to.read()(which I'm not sure if it's really it's responsibility, but for a future issue), and accessing the attributes of the class.Check List
CONTRIBUTING.md.pyproject.toml.CHANGELOG.mdif this is a change that can affect users.Closes #1234).