Skip to content

Conversation

@zmaalick
Copy link
Collaborator

@zmaalick zmaalick commented Jan 8, 2026

Closes #165 .

PR creation checklist for the developer

  • Has <issue_number> above ☝️ been replaced with the issue number?
  • Has main been selected as the base branch?
  • Does the feature branch name follow the format <issue_number>_<short_description_of_feature>?
  • Does the text of the PR title exactly match with the text (not including the issue number) of the issue title?
  • Have appropriate reviewers been added to the PR (once it is ready for review)?
  • Has the PR been assigned to the developer(s)?
  • Have the same labels as on the issue (except for the good first issue label) been added to the PR?
  • Has the Climate Model Evaluation Workflow (CMEW) project been added to the PR?
  • Has the appropriate milestone been added to the PR?

Definition of Done for the developer

  • Does the change in this PR address the above issue / have all acceptance criteria been met?
  • Does the change in this PR follow the requirements in the wiki: Developer Guide (including copyrights)?
  • Have new tests related to the change been added?
  • Do all the GitHub workflow checks pass?
  • Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
  • Has the API documentation (e.g. docstrings in Python modules) related to the change been updated appropriately?
  • Has the user documentation (i.e. everything in the doc directory) related to the change been updated appropriately, including the Quick Start section?
  • Do the HTML pages render correctly? (See wiki: Build the documentation locally)

PR creation checklist for the reviewer

  • Has <issue_number> above ☝️ been replaced with the issue number?
  • Has main been selected as the base branch?
  • Does the feature branch name follow the format <issue_number>_<short_description_of_feature>?
  • Does the text of the PR title exactly match with the text (not including the issue number) of the issue title?
  • Have appropriate reviewers been added to the PR (once it is ready for review)?
  • Has the PR been assigned to the developer(s)?
  • Have the same labels as on the issue (except for the good first issue label) been added to the PR?
  • Has the Climate Model Evaluation Workflow (CMEW) project been added to the PR?
  • Has the appropriate milestone been added to the PR?

Definition of Done for the reviewer

  • Does the change in this PR address the above issue / have all acceptance criteria been met?
  • Does the change in this PR follow the requirements in the wiki: Developer Guide (including copyrights)?
  • Have new tests related to the change been added? N/A
  • Do all the GitHub workflow checks pass?
  • Do all the tests run locally and pass? (Note: the tests are not run by the GitHub workflow, see wiki: Run the tests locally)
  • Has the API documentation (e.g. docstrings in Python modules) related to the change been updated appropriately? N/A
  • Has the user documentation (i.e. everything in the doc directory) related to the change been updated appropriately including the Quick Start section? N/A
  • Do the HTML pages render correctly? (See wiki: Build the documentation locally)

@zmaalick zmaalick self-assigned this Jan 8, 2026
@zmaalick zmaalick added good first issue Good for newcomers quality assurance Anything related to Quality Assurance (QA) testing Anything related to testing labels Jan 8, 2026
@zmaalick zmaalick added this to the v0.2.0 (multiple model runs) milestone Jan 8, 2026
Copy link
Collaborator

@mo-nikosbaltas mo-nikosbaltas left a comment

Choose a reason for hiding this comment

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

Although the unit tests block is correct and works, my suggestion is:
Instead of:

eval "$(conda shell.bash hook)"
conda activate cmew
...

Use conda run:
conda run -n cmew cylc validate -O metoffice -O unittest .

This eliminates shell activation issues and is more reliable in Actions.
This also applies to other tests in this file.
As already said this is a suggestion.

     - name: Run Cylc unit tests
        run: |
          cd CMEW
          conda run -n cmew cylc vip -O metoffice -O unittest .

Although not applicable for this issue I would suggest to include some logging by adding at the end:

      - name: Upload cylc-run logs on failure
        if: failure()
        uses: actions/upload-artifact@v4
        with:
          name: cylc-run-logs
          path: ~/cylc-run

Otherwise it is all good for me.

@mo-nikosbaltas
Copy link
Collaborator

The unit tests should read:

     - name: Run Cylc unit tests
        run: |
          cd CMEW
          conda run -n cmew cylc vip -O metoffice -O unittest .

not
conda run -n cmew cylc validate -O metoffice -O unittest .

In my previous comment I was giving an example for 'cylc validate'. Sorry if I confused you. Also the checks failed when pushing. Can you correct?

@zmaalick
Copy link
Collaborator Author

zmaalick commented Jan 9, 2026

Although the unit tests block is correct and works, my suggestion is: Instead of:

eval "$(conda shell.bash hook)"
conda activate cmew
...

Use conda run: conda run -n cmew cylc validate -O metoffice -O unittest .

This eliminates shell activation issues and is more reliable in Actions. This also applies to other tests in this file. As already said this is a suggestion.

     - name: Run Cylc unit tests
        run: |
          cd CMEW
          conda run -n cmew cylc vip -O metoffice -O unittest .

Although not applicable for this issue I would suggest to include some logging by adding at the end:

      - name: Upload cylc-run logs on failure
        if: failure()
        uses: actions/upload-artifact@v4
        with:
          name: cylc-run-logs
          path: ~/cylc-run

Otherwise it is all good for me.

Made the changes and also added the logging. Working fine.

@mo-nikosbaltas mo-nikosbaltas self-requested a review January 12, 2026 12:06
Copy link
Collaborator

@mo-nikosbaltas mo-nikosbaltas left a comment

Choose a reason for hiding this comment

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

Modify PR title to : #321: Add running the unittests to the GitHub workflow. See https://github.com/MetOffice/CMEW/wiki/Detailed-Working-Practices#create-a-pull-request

@zmaalick zmaalick changed the title add unitests in github workflow Add running the unittests to the GitHub workflow Jan 12, 2026
@zmaalick
Copy link
Collaborator Author

Modify PR title to : #321: Add running the unittests to the GitHub workflow. See https://github.com/MetOffice/CMEW/wiki/Detailed-Working-Practices#create-a-pull-request

Done

@mo-nikosbaltas
Copy link
Collaborator

Also, @zmaalick can you change the copyright years (# (C) Crown Copyright 2022-2025, Met Office.) in the file to 2022-2026.

@mo-nikosbaltas mo-nikosbaltas self-requested a review January 12, 2026 13:30
@zmaalick
Copy link
Collaborator Author

Also, @zmaalick can you change the copyright years (# (C) Crown Copyright 2022-2025, Met Office.) in the file to 2022-2026.

Done

mo-nikosbaltas
mo-nikosbaltas previously approved these changes Jan 12, 2026
Copy link
Collaborator

@mo-nikosbaltas mo-nikosbaltas left a comment

Choose a reason for hiding this comment

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

All looks good.

@mo-nikosbaltas mo-nikosbaltas self-requested a review January 12, 2026 13:42
Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks @zmaalick 😊

The issue states "It may be worth configuring pytest so that it can be run from the GitHub workflow and via -O unittest without needing to change directories"; would it be possible for you to look into this, please? 👍

@alistairsellar alistairsellar requested review from alistairsellar and removed request for alistairsellar January 13, 2026 16:35
Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks @zmaalick! 🥳

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks @zmaalick 🥳

The "end of new line" issue with one of the files in this branch suggests you have not set up the pre-commit hooks. Please set up the pre-commit hooks. Instructions are available at https://github.com/MetOffice/CMEW/wiki/First-time-developer-instructions#first-time-developer-instructions.

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Thanks @zmaalick 😊

Now that the pytest configuration has been added, please remove the cd command to be removed from the unittest app.

# (C) Crown Copyright 2024-2025, Met Office.
# The LICENSE.md file contains full licensing details.
import os

Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This change still needs reverting 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

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

Almost there! 🤪

# (C) Crown Copyright 2024-2025, Met Office.
# The LICENSE.md file contains full licensing details.
import os

Copy link
Member

Choose a reason for hiding this comment

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

This change still needs reverting 😊

@@ -1,5 +1,5 @@
#!/usr/bin/env python
# (C) Crown Copyright 2024-2025, Met Office.
# (C) Crown Copyright 2024-2026, Met Office.
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change (there are now no changes in this file, so the copyright doesn't need updating) 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@mo-nikosbaltas mo-nikosbaltas left a comment

Choose a reason for hiding this comment

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

Looks ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

quality assurance Anything related to Quality Assurance (QA) testing Anything related to testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add running the unittests to the GitHub workflow

5 participants