Skip to content

Conversation

@samsrabin
Copy link
Member

@samsrabin samsrabin commented Jun 5, 2025

Description of changes

Breaks up the "omnibus docs test" GitHub Workflow to minimize the number of workflow runs. Also solves problem where people were getting test failures on their forks.

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed: None

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any:

  • Local running of testing.sh
  • Actions on this PR

@samsrabin samsrabin self-assigned this Jun 5, 2025
@samsrabin samsrabin added documentation additions or edits to user-facing documentation or its infrastructure bfb bit-for-bit devops Development Operations to improve development throughput, E.g., adding GitHub Workflows docs:infrastructure New features or bug fixes in the documentation infrastructure docs-loc:infrastructure Relates to documentation infrastructure labels Jun 5, 2025
@samsrabin samsrabin marked this pull request as ready for review June 5, 2025 22:22
@samsrabin samsrabin requested a review from ekluzek June 5, 2025 22:22
@samsrabin samsrabin changed the title Break up omnibus docs test Clean up docs workflows Jun 5, 2025
@samsrabin samsrabin force-pushed the break-up-omnibus-docs-test branch from 74cd8fa to d5b7f1e Compare June 6, 2025 15:20
@samsrabin samsrabin added the PR status: awaiting review Work on this PR is paused while waiting for review. label Jun 9, 2025
# Conflicts:
#	.github/workflows/docs-omnibus.yml
#	doc/testing.sh
@samsrabin samsrabin moved this to In progress - b4b-dev in CTSM: Upcoming tags Jun 23, 2025
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

@samsrabin thanks so much for your work here. This is a lot of improvement, breaking things up to isolate complexity, and formalizing of the testing for docs, which is really great to bring in.

The reason I put the request changes, button is that you will need to trap for errors in the bash testing. I think a lot of the tests will continue with errors, or at least not have the clearest kind of error handling for developers to see what to do to fix a test. So I see that as critical to make sure is working well.

It also made me realize we need to think about how we should do our testing for bash. So I'll open a discussion on that, and we'll talk about it at a future Thursday meeting. Most of that is outside the scope here, but a good thing to be thinking about.

Some small things I suggest:

  • Change the name of doc/testing to doc/test
  • Change "Yep!" to something like ... "Successfully compared that the two methods give identical results!"
  • Consider changing the filenames with a dash option in the name
  • Ask for a few comments in a few places

I also wonder if more things could be added into the common test part, but that can be tried later. This gives a structure to facilitate doing that sort of thing, so doesn't need to happen here.

Finally, what are the use cases for supporting the three build methods: makefile, ctsm_pylib, and doc-builder? Are all important to maintain? Could we drop any of them? If not this testing will be critical in order to support all of them. And we can assess this question outside of this PR, and do that later, as we need to think about this and discuss before we do anything about it.

@github-project-automation github-project-automation bot moved this from In progress - b4b-dev to Stalled (needs review, blocked etc.) in CTSM: Upcoming tags Jun 23, 2025
@samsrabin samsrabin removed next this should get some attention in the next week or two. Normally each Thursday SE meeting. PR status: awaiting review Work on this PR is paused while waiting for review. labels Jun 24, 2025
Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

Marking as approve now. I still have a few recommendations to do, but they are mostly in the space of adding comments.

I was concerned about the lack of error trapping, but the use of "set -e" will suffice for now. Longer term we should move to using error trapping for at least assert type testing. This allows for nice messaging on the problem and for the test script to continue afterwards.

@github-project-automation github-project-automation bot moved this from Stalled (needs review, blocked etc.) to In progress - master in CTSM: Upcoming tags Jun 24, 2025
@samsrabin samsrabin merged commit 2408ba6 into ESCOMP:b4b-dev Jun 24, 2025
16 checks passed
@github-project-automation github-project-automation bot moved this from In progress - master to Done (non release/external) in CTSM: Upcoming tags Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bfb bit-for-bit devops Development Operations to improve development throughput, E.g., adding GitHub Workflows docs:infrastructure New features or bug fixes in the documentation infrastructure docs-loc:infrastructure Relates to documentation infrastructure documentation additions or edits to user-facing documentation or its infrastructure

Projects

Status: Done (non release/external)

Development

Successfully merging this pull request may close these issues.

Break up "omnibus docs test" Need to add the release-clm5.0 branch to your fork for the docs build tests to work

2 participants