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

add pyyaml to test_require #2047

Merged
merged 1 commit into from
Dec 27, 2024
Merged

add pyyaml to test_require #2047

merged 1 commit into from
Dec 27, 2024

Conversation

LuiggiTenorioK
Copy link
Member

@LuiggiTenorioK LuiggiTenorioK commented Dec 23, 2024

PR to fix our CI pipeline because pyyaml is not installed indirectly anymore by one of our dependencies and is used inside test_rocrate.py.

I tried to change it to ruamel.yaml by doing:

diff --git a/test/unit/provenance/test_rocrate.py b/test/unit/provenance/test_rocrate.py
index 8e0b4e1c..134c4190 100644
--- a/test/unit/provenance/test_rocrate.py
+++ b/test/unit/provenance/test_rocrate.py
@@ -6,7 +6,8 @@ from subprocess import CalledProcessError
 from tempfile import TemporaryDirectory
 from unittest import TestCase
 
-import yaml
+from io import StringIO
+from ruamel.yaml import YAML
 from mock import Mock, patch
 from rocrate.rocrate import File
 from rocrate.rocrate import ROCrate
@@ -48,7 +49,10 @@ class TestRoCrate(TestCase):
         Path(conf_dir, 'metadata').mkdir()
         unified_config = Path(conf_dir, 'metadata/experiment_data.yml')
         unified_config.touch()
-        unified_config.write_text(yaml.dump(as_conf.experiment_data))
+        stream = StringIO()
+        YAML().dump(as_conf.experiment_data, stream)
+        yaml_string = stream.getvalue()
+        unified_config.write_text(yaml_string)
         as_conf.current_loaded_files = {unified_config: 0}
 
     def test_add_dir_and_files_empty_folder(self):

but ruaml.yaml prevents you to write a in the file with an arbitrary object type which is necessary for the TestRoCrate::test_rocrate_invalid_parameter_type test. Alternatively, I added pyyaml to the test_require as the easiest solution.

@LuiggiTenorioK LuiggiTenorioK self-assigned this Dec 23, 2024
@LuiggiTenorioK LuiggiTenorioK marked this pull request as ready for review December 23, 2024 13:59
@codecov-commenter
Copy link

codecov-commenter commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.89%. Comparing base (3706cb9) to head (c25114e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2047      +/-   ##
==========================================
- Coverage   48.92%   48.89%   -0.04%     
==========================================
  Files          72       72              
  Lines       17543    17543              
  Branches     3415     3415              
==========================================
- Hits         8583     8577       -6     
- Misses       8156     8158       +2     
- Partials      804      808       +4     
Flag Coverage Δ
fast-tests 48.89% <ø> (-0.04%) ⬇️

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.

@LuiggiTenorioK LuiggiTenorioK requested a review from kinow December 23, 2024 13:59
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reproduced the error in the pipeline that @isimo00 had, and confirmed that this fixes it.

But maybe we can use ruamel.yaml instead. The test is intentionally providing an object that won't work with ruamel.yaml, and the idea of the test is to make sure that when that happens the RO-Crate is not created.

But ruamel gives the error sooner than with pyyaml, so I think we can modify the code to handle it like this (patched against this PR, after pip uninstall pyyaml)?

 diff --git a/setup.py b/setup.py
index 11166312..3cee1642 100644
--- a/setup.py
+++ b/setup.py
@@ -69,7 +69,6 @@ tests_require = [
     'pytest-cov',
     'pytest-mock',
     'ruff==0.6.2',
-    'pyyaml',
     # 'testcontainers'  # TODO: pending Postgres MR
 ]
 
diff --git a/test/unit/provenance/test_rocrate.py b/test/unit/provenance/test_rocrate.py
index 8e0b4e1c..a2a6544b 100644
--- a/test/unit/provenance/test_rocrate.py
+++ b/test/unit/provenance/test_rocrate.py
@@ -6,10 +6,11 @@ from subprocess import CalledProcessError
 from tempfile import TemporaryDirectory
 from unittest import TestCase
 
-import yaml
 from mock import Mock, patch
 from rocrate.rocrate import File
 from rocrate.rocrate import ROCrate
+from ruamel.yaml import YAML
+from ruamel.yaml.representer import RepresenterError
 
 from autosubmit.autosubmit import Autosubmit
 from autosubmit.job.job import Job
@@ -48,7 +49,9 @@ class TestRoCrate(TestCase):
         Path(conf_dir, 'metadata').mkdir()
         unified_config = Path(conf_dir, 'metadata/experiment_data.yml')
         unified_config.touch()
-        unified_config.write_text(yaml.dump(as_conf.experiment_data))
+        yaml = YAML(typ='rt')
+        with open(unified_config, 'w') as f:
+            yaml.dump(dict(as_conf.experiment_data), f)
         as_conf.current_loaded_files = {unified_config: 0}
 
     def test_add_dir_and_files_empty_folder(self):
@@ -428,26 +431,16 @@ class TestRoCrate(TestCase):
                     'APP'
                 ]
             }
-            self._create_conf_dir(experiment_path)
-            jobs = []
 
             mocked_get_autosubmit_version.return_value = '4.0.0b0'
             mocked_get_experiment_descrip.return_value = [
                 ['mocked test project']
             ]
 
-            with self.assertRaises(AutosubmitCritical) as cm:
-                create_rocrate_archive(
-                    as_conf=self.as_conf,
-                    rocrate_json=rocrate_json,
-                    jobs=jobs,
-                    start_time=None,
-                    end_time=None,
-                    path=Path(temp_dir)
-                )
+            with self.assertRaises(RepresenterError) as cm:
+                self._create_conf_dir(experiment_path)
 
-            self.assertEqual(cm.exception.message,
-                              'Could not locate a type in RO-Crate for parameter APP.OBJ type object')
+            self.assertTrue('cannot represent an object' in str(cm.exception))
 
     @patch('autosubmit.autosubmit.Log')
     @patch('autosubmit.autosubmit.AutosubmitConfig')

That should fix the pipeline without requiring the pyyaml test dependency, I think.

@kinow
Copy link
Member

kinow commented Dec 27, 2024

(And my bad for using yaml there in the first place!)

@LuiggiTenorioK
Copy link
Member Author

Thanks @kinow for the patch confirming how to modify that test. It has been applied and it works! Feel free to rebase it.

@kinow
Copy link
Member

kinow commented Dec 27, 2024

Thank you Luiggi! Ready to be merged!

@LuiggiTenorioK LuiggiTenorioK merged commit 0ad5a34 into master Dec 27, 2024
8 checks passed
@LuiggiTenorioK LuiggiTenorioK deleted the hotfix-ci-pyyaml branch December 27, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants